[WIP] [persistence] Store the time of the update not the time the store occurred - #21160
[WIP] [persistence] Store the time of the update not the time the store occurred#21160mjagdis wants to merge 10 commits into
Conversation
Signed-off-by: Mike Jagdis <mjagdis@eris-associates.co.uk>
Signed-off-by: Mike Jagdis <mjagdis@eris-associates.co.uk>
Times stored to dynamoDB are Dates which only have millisecond precision so we need to truncate ZonedDateTime.now to milliseconds as well otherwise time can seem to go backwards if actions occur within the same millisecond (as they can easily do under testing). Since actions can occur within the same millisecond all the isBefore and isAfter tests are wrong - the times can be equal as well. Even so, there are some tests that assume times cannot be equal. The simple workaround is to add Thread.sleep(1) (to go with the existing Thread.sleep(10)) Signed-off-by: Mike Jagdis <mjagdis@eris-associates.co.uk>
Signed-off-by: Mike Jagdis <mjagdis@eris-associates.co.uk>
Signed-off-by: Mike Jagdis <mjagdis@eris-associates.co.uk>
Signed-off-by: Mike Jagdis <mjagdis@eris-associates.co.uk>
Signed-off-by: Mike Jagdis <mjagdis@eris-associates.co.uk>
Signed-off-by: Mike Jagdis <mjagdis@eris-associates.co.uk>
…urred Signed-off-by: Mike Jagdis <mjagdis@eris-associates.co.uk>
| assertStateEquals(getFirstItemState(), storedFirst.getState()); | ||
| assertTrue(storedFirst.getTimestamp().toInstant().isBefore(afterStore1.toInstant())); | ||
| assertTrue(storedFirst.getTimestamp().toInstant().isAfter(beforeStore.toInstant())); | ||
| assertTrue(!storedFirst.getTimestamp().toInstant().isAfter(afterStore1.toInstant())); |
There was a problem hiding this comment.
Should we instead have sleep (1ms) somewhere?
I think these tests are mainly asserting ordering. If everything would be equal, that would not expected either?
| } else if (begin != null && end == null) { | ||
| queryBuilder.queryConditional(QueryConditional | ||
| .sortGreaterThan(k -> k.partitionValue(partition).sortValue(timeConverter.transformFrom(begin)))); | ||
| queryBuilder.queryConditional(QueryConditional.sortGreaterThanOrEqualTo( |
| @@ -496,7 +496,8 @@ public void store(Item item) { | |||
| @Override | |||
| public void store(Item item, @Nullable String alias) { | |||
| // Timestamp and capture state immediately as rest of the store is asynchronous (state might change in between) | |||
| ZonedDateTime time = ZonedDateTime.now(); | |||
| ZonedDateTime lastStateUpdate = item.getLastStateUpdate(); | |||
There was a problem hiding this comment.
Is it guaranteed by the core that this timestamp is updated when persistence is called?
There was a problem hiding this comment.
Yes. If it isn't mapdb would have been badly broken for a long time and that would be Bad. It would also be a bug in the core since state and update time should change together. (Some might argue that the applies-from time should be part of the state...)
There was a problem hiding this comment.
Hmm what do you think about this AI analysis?
https://claude.ai/share/009bca45-734b-4c29-9d25-eac0eba4291f
I was worried as so much of this happens asynchronously in corr
There was a problem hiding this comment.
It looks like a core bug to me. I'd guess the chances of one of the notify or event jobs getting scheduled, starting up and making "enough" progress before applyState sets the update time is pretty slim. But it does seem wrong.
See Core issue 5711 and PR#5712
| queryBuilder.queryConditional(QueryConditional.sortBetween( | ||
| k -> k.partitionValue(partition).sortValue(timeConverter.transformFrom(begin)), | ||
| k -> k.partitionValue(partition).sortValue(timeConverter.transformFrom(end)))); | ||
| k -> k.partitionValue(partition).sortValue(timeConverter.transformFrom(begin.minusNanos(1))), |
There was a problem hiding this comment.
Hmm should it be Nanos or millis given the accuracy in persistence
There was a problem hiding this comment.
Noooo... I think we need to truncate to milliseconds first and then add/sub a nanosecond to make the range inclusive.
Truncate to milliseconds to match update times before nudging by a nanosecond to make the range inclusive. Signed-off-by: Mike Jagdis <mjagdis@eris-associates.co.uk>
|
Additional changes (including core) are needed in order to handle periodic stores (cron-type strategies). |
There was a problem hiding this comment.
Pull request overview
This PR aims to make persistence timestamps reflect the item’s last state update time (when available) instead of “time of store”, so different persistence add-ons agree on event time and avoid bogus roll-forward entries.
Changes:
- Use
item.getLastStateUpdate()(fallback to “now”) as the timestamp in multiple persistence services. - Adjust DynamoDB query time bounds and update DynamoDB integration tests to be robust with millisecond timestamp precision.
- Minor refactors to route
store(Item)throughstore(Item, @Nullable String alias)implementations.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| bundles/org.openhab.persistence.timescaledb/src/main/java/org/openhab/persistence/timescaledb/internal/TimescaleDBPersistenceService.java | Store using item last state update time |
| bundles/org.openhab.persistence.rrd4j/src/main/java/org/openhab/persistence/rrd4j/internal/RRD4jPersistenceService.java | Use last state update epoch-second as timestamp |
| bundles/org.openhab.persistence.mongodb/src/main/java/org/openhab/persistence/mongodb/internal/MongoDBPersistenceService.java | Store using last state update (fallback now) |
| bundles/org.openhab.persistence.jpa/src/main/java/org/openhab/persistence/jpa/internal/JpaPersistenceService.java | Persist timestamp from last state update when present |
| bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/JdbcPersistenceService.java | Capture state + last update time before async store |
| bundles/org.openhab.persistence.inmemory/src/main/java/org/openhab/persistence/inmemory/internal/InMemoryPersistenceService.java | Use last state update for in-memory timestamps |
| bundles/org.openhab.persistence.dynamodb/src/main/java/org/openhab/persistence/dynamodb/internal/DynamoDBPersistenceService.java | Use item last state update for persisted timestamp |
| bundles/org.openhab.persistence.dynamodb/src/main/java/org/openhab/persistence/dynamodb/internal/DynamoDBQueryUtils.java | Make DynamoDB time filtering inclusive / ms-aware |
| bundles/org.openhab.persistence.dynamodb/src/test/java/org/openhab/persistence/dynamodb/internal/SwitchItemIntegrationTest.java | Align test timing to ms precision |
| bundles/org.openhab.persistence.dynamodb/src/test/java/org/openhab/persistence/dynamodb/internal/StringItemIntegrationTest.java | Align test timing to ms precision |
| bundles/org.openhab.persistence.dynamodb/src/test/java/org/openhab/persistence/dynamodb/internal/RollershutterItemIntegrationTest.java | Align test timing to ms precision |
| bundles/org.openhab.persistence.dynamodb/src/test/java/org/openhab/persistence/dynamodb/internal/PlayerItemRewindFastForwardIntegrationTest.java | Align test timing to ms precision |
| bundles/org.openhab.persistence.dynamodb/src/test/java/org/openhab/persistence/dynamodb/internal/PlayerItemPlayPauseIntegrationTest.java | Align test timing to ms precision |
| bundles/org.openhab.persistence.dynamodb/src/test/java/org/openhab/persistence/dynamodb/internal/PagingIntegrationTest.java | Adjust paging test start time to ms precision |
| bundles/org.openhab.persistence.dynamodb/src/test/java/org/openhab/persistence/dynamodb/internal/NumberItemIntegrationTest.java | Align test timing to ms precision |
| bundles/org.openhab.persistence.dynamodb/src/test/java/org/openhab/persistence/dynamodb/internal/LocationItemIntegrationTest.java | Align test timing to ms precision |
| bundles/org.openhab.persistence.dynamodb/src/test/java/org/openhab/persistence/dynamodb/internal/DimmerItemIntegrationTest.java | Align test timing to ms precision |
| bundles/org.openhab.persistence.dynamodb/src/test/java/org/openhab/persistence/dynamodb/internal/DateTimeItemIntegrationTest.java | Align test timing to ms precision |
| bundles/org.openhab.persistence.dynamodb/src/test/java/org/openhab/persistence/dynamodb/internal/ContactItemIntegrationTest.java | Align test timing to ms precision |
| bundles/org.openhab.persistence.dynamodb/src/test/java/org/openhab/persistence/dynamodb/internal/ColorItemIntegrationTest.java | Align test timing to ms precision |
| bundles/org.openhab.persistence.dynamodb/src/test/java/org/openhab/persistence/dynamodb/internal/CallItemIntegrationTest.java | Align test timing to ms precision |
| bundles/org.openhab.persistence.dynamodb/src/test/java/org/openhab/persistence/dynamodb/internal/AbstractTwoItemIntegrationTest.java | Make timestamp assertions inclusive of boundaries |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| queryBuilder.queryConditional(QueryConditional.sortBetween( | ||
| k -> k.partitionValue(partition).sortValue(timeConverter.transformFrom(begin)), | ||
| k -> k.partitionValue(partition).sortValue(timeConverter.transformFrom(end)))); | ||
| k -> k.partitionValue(partition) | ||
| .sortValue(timeConverter.transformFrom(begin.truncatedTo(ChronoUnit.MILLIS).minusNanos(1))), | ||
| k -> k.partitionValue(partition) | ||
| .sortValue(timeConverter.transformFrom(end.truncatedTo(ChronoUnit.MILLIS).plusNanos(1))))); |
| try { | ||
| Thread.sleep(1); | ||
| } catch (InterruptedException e) { | ||
| } |
Only mapdb has it right. The rest... :-(
Using now instead of the update time means (a) different persistence services disagree about when state update occurred and (b) roll-forward of forecast time series introduces bogus entries in persistence.