Pass timestamps from Timeseries to ModifiablePersistenceServices - #5733
Pass timestamps from Timeseries to ModifiablePersistenceServices#5733mjagdis wants to merge 3 commits into
Conversation
Signed-off-by: Mike Jagdis <mjagdis@eris-associates.co.uk>
|
While this could be a quick fix, why not make it more general and open the way to improve the addons from here. I would suggest the following:
|
Signed-off-by: Mike Jagdis <mjagdis@eris-associates.co.uk>
|
How about this? |
| long startTime = System.nanoTime(); | ||
| itemConfig.filters().forEach(filter -> filter.persisted(item)); | ||
| persistenceService.store(item, getAlias(item)); | ||
| persistenceService.store(item, now, item.getState(), getAlias(item)); |
There was a problem hiding this comment.
If an item's state changes while iterating over the items, the date will be wrong. You should only apply this fix when restoring forecasts.
There was a problem hiding this comment.
Try this then. There always is a risk that the state can change under us because GenericItem doesn't seem to have any atomicity guarantees but this should be no worse than previously. Possibly slightly better since now the core ensures now and getState are always next to each other whereas before it depended how the service implemented store.
Signed-off-by: Mike Jagdis <mjagdis@eris-associates.co.uk>
| itemConfig.filters().forEach(filter -> filter.persisted(item)); | ||
| container.getPersistenceService().store(item, container.getAlias(item)); | ||
| PersistenceService persistenceService = container.getPersistenceService(); | ||
| persistenceService.store(item, Objects.requireNonNullElse(item.getLastStateUpdate(), now), |
There was a problem hiding this comment.
lastStateUpdate is not guaranteed to be updated when this is called. It will likely be as it runs in a separate thread, but applyState in GenericItem explicitly only updates it after the notifyListeners call. So I am not sure this is always valid.
| void store(Item item); | ||
| @Deprecated | ||
| default void store(Item item) { | ||
| store(item, ZonedDateTime.now(), item.getState(), null); |
There was a problem hiding this comment.
Make these NOOP, or you end up in a circular reference if nothing is implemented.
There was a problem hiding this comment.
Pull request overview
This PR updates the persistence API and persistence manager flow so that a concrete timestamp/state can be forwarded into persistence services (instead of each service deriving its own “now”), improving correctness for backfilled/time-series driven updates.
Changes:
- Extend
PersistenceServicewith timestamp/state-awarestore(...)overloads and migrate call sites to use them. - Remove duplicate historic
store(...)declarations fromModifiablePersistenceService(now inherited fromPersistenceService). - Update persistence manager tests to verify the new
store(...)method signature is used.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| bundles/org.openhab.core.persistence/src/test/java/org/openhab/core/persistence/internal/PersistenceManagerTest.java | Updates Mockito verifications to expect timestamp/state-aware store(...) calls. |
| bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/PersistenceService.java | Introduces timestamp/state-aware store(...) overloads and deprecates the legacy methods. |
| bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/ModifiablePersistenceService.java | Removes now-redundant historic store(...) method declarations. |
| bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/internal/PersistenceManagerImpl.java | Switches persistence writes to pass explicit timestamps/states through to PersistenceService. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Deprecated | ||
| default void store(Item item, @Nullable String alias) { | ||
| store(item, ZonedDateTime.now(), item.getState(), alias); | ||
| } |
| * @param item the data to be stored | ||
| * @param date the date of the record | ||
| * @param state the state to be recorded | ||
| */ |
|
|
||
| // Check if other persistence services got updated | ||
| verify(persistenceServiceMock).store(TEST_ITEM, null); | ||
| verify(persistenceServiceMock).store(eq(TEST_ITEM), any(ZonedDateTime.class), any(State.class), eq(null)); |
wborn
left a comment
wborn
left a comment
There was a problem hiding this comment.
AI reviewed the current PR HEAD and the existing review discussion.
The general approach of adding timestamp/state-aware store(...) overloads to PersistenceService, while allowing existing persistence services to fall back to their current implementation, looks reasonable.
There are still some unresolved issues that should be addressed before merging:
- As already pointed out in the existing review,
item.getLastStateUpdate()is not reliable for normal state callbacks becauseGenericItemupdates it after notifying listeners. The timestamp from the restored time-series entry should only be propagated along paths where that timestamp is actually known. - The current default
store(...)methods can recurse indefinitely when an implementation overrides neither side of the compatibility bridge. - The time-series test should verify the actual timestamp and state forwarded to the other persistence service instead of accepting any values.
- The missing
@param aliasJavadoc should be added.
There is also one related API migration that should be included here: PersistenceExtensions.internalPersist(Item, ...) still calls the now-deprecated store(item, alias) overload. Since this PR establishes the timestamp/state overload as the replacement, core should use the new API itself. This is particularly important if the deprecated defaults are made no-ops to resolve the recursion issue.
Another maintainer should still review the PR after the AI review and the remaining issues have been addressed.
| */ | ||
| void store(Item item, @Nullable String alias); | ||
| @Deprecated | ||
| default void store(Item item, @Nullable String alias) { |
There was a problem hiding this comment.
PersistenceExtensions.internalPersist(Item, ...) still calls this overload. Since this PR deprecates it in favor of the timestamp/state-aware method, can we migrate that core caller as part of this change as well?
This becomes particularly important if these deprecated defaults are made no-ops to resolve the recursion discussed above: a persistence service implementing only the new API would otherwise silently ignore persist(item) calls. Using the new overload there with ZonedDateTime.now() and item.getState() should preserve the existing semantics while keeping the compatibility delegation one-directional.
Pass the timestamps given in Timeseries through to ModifiablePersistenceServices rather than just letting them pick their own and independent idea of "now".
N.B. It would be nice to have correct timestamps in QueryablePersistenceServices too but they don't have the extended store method and since it's an interface hierarchy rather than a class hierarchy fixing that might take a bit of coordination between core and addons. I don't think it's desperately urgent for the queryables...