-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[WIP] [persistence] Store the time of the update not the time the store occurred #21160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
491da36
5881b35
f3990e4
94d977d
15ec1c7
4737949
e669fa1
3a4ad70
8a2819b
5ef2eee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |
|
|
||
| import java.lang.reflect.InvocationTargetException; | ||
| import java.time.ZonedDateTime; | ||
| import java.time.temporal.ChronoUnit; | ||
|
|
||
| import org.eclipse.jdt.annotation.NonNullByDefault; | ||
| import org.eclipse.jdt.annotation.Nullable; | ||
|
|
@@ -194,15 +195,17 @@ private static void addFilterbyItemAndTimeFilter(QueryEnhancedRequest.Builder qu | |
| // No need to place time filter, but we do filter by partition | ||
| queryBuilder.queryConditional(QueryConditional.keyEqualTo(k -> k.partitionValue(partition))); | ||
| } else if (begin != null && end == null) { | ||
| queryBuilder.queryConditional(QueryConditional | ||
| .sortGreaterThan(k -> k.partitionValue(partition).sortValue(timeConverter.transformFrom(begin)))); | ||
| queryBuilder.queryConditional(QueryConditional.sortGreaterThanOrEqualTo( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice find! |
||
| k -> k.partitionValue(partition).sortValue(timeConverter.transformFrom(begin)))); | ||
| } else if (begin == null && end != null) { | ||
| queryBuilder.queryConditional(QueryConditional | ||
| .sortLessThan(k -> k.partitionValue(partition).sortValue(timeConverter.transformFrom(end)))); | ||
| queryBuilder.queryConditional(QueryConditional.sortLessThanOrEqualTo( | ||
| k -> k.partitionValue(partition).sortValue(timeConverter.transformFrom(end)))); | ||
| } else if (begin != null && end != null) { | ||
| 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))))); | ||
|
Comment on lines
204
to
+208
|
||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -110,12 +110,12 @@ protected void assertIterableContainsItems(Iterable<HistoricItem> iterable, bool | |
| } | ||
|
|
||
| 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())); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| assertTrue(!storedFirst.getTimestamp().toInstant().isBefore(beforeStore.toInstant())); | ||
|
|
||
| assertStateEquals(getSecondItemState(), storedSecond.getState()); | ||
| assertTrue(storedSecond.getTimestamp().toInstant().isBefore(afterStore2.toInstant())); | ||
| assertTrue(storedSecond.getTimestamp().toInstant().isAfter(afterStore1.toInstant())); | ||
| assertTrue(!storedSecond.getTimestamp().toInstant().isAfter(afterStore2.toInstant())); | ||
| assertTrue(!storedSecond.getTimestamp().toInstant().isBefore(afterStore1.toInstant())); | ||
| } | ||
|
|
||
| @Test | ||
|
|
@@ -225,8 +225,8 @@ public void testQueryUsingNameAndStartAndEndWithNEQOperator() { | |
| HistoricItem actual1 = iterator.next(); | ||
| assertFalse(iterator.hasNext()); | ||
| assertStateEquals(getFirstItemState(), actual1.getState()); | ||
| assertTrue(actual1.getInstant().isBefore(afterStore1.toInstant())); | ||
| assertTrue(actual1.getInstant().isAfter(beforeStore.toInstant())); | ||
| assertTrue(!actual1.getInstant().isAfter(afterStore1.toInstant())); | ||
| assertTrue(!actual1.getInstant().isBefore(beforeStore.toInstant())); | ||
| }); | ||
| } | ||
|
|
||
|
|
@@ -246,8 +246,8 @@ public void testQueryUsingNameAndStartAndEndWithEQOperator() { | |
| HistoricItem actual1 = iterator.next(); | ||
| assertFalse(iterator.hasNext()); | ||
| assertStateEquals(getFirstItemState(), actual1.getState()); | ||
| assertTrue(actual1.getInstant().isBefore(afterStore1.toInstant())); | ||
| assertTrue(actual1.getInstant().isAfter(beforeStore.toInstant())); | ||
| assertTrue(!actual1.getInstant().isAfter(afterStore1.toInstant())); | ||
| assertTrue(!actual1.getInstant().isBefore(beforeStore.toInstant())); | ||
| }); | ||
| } | ||
|
|
||
|
|
@@ -267,8 +267,8 @@ public void testQueryUsingNameAndStartAndEndWithLTOperator() { | |
| HistoricItem actual1 = iterator.next(); | ||
| assertFalse(iterator.hasNext()); | ||
| assertStateEquals(getFirstItemState(), actual1.getState()); | ||
| assertTrue(actual1.getInstant().isBefore(afterStore1.toInstant())); | ||
| assertTrue(actual1.getInstant().isAfter(beforeStore.toInstant())); | ||
| assertTrue(!actual1.getInstant().isAfter(afterStore1.toInstant())); | ||
| assertTrue(!actual1.getInstant().isBefore(beforeStore.toInstant())); | ||
| }); | ||
| } | ||
|
|
||
|
|
@@ -304,8 +304,8 @@ public void testQueryUsingNameAndStartAndEndWithLTEOperator() { | |
| HistoricItem actual1 = iterator.next(); | ||
| assertFalse(iterator.hasNext()); | ||
| assertStateEquals(getFirstItemState(), actual1.getState()); | ||
| assertTrue(actual1.getInstant().isBefore(afterStore1.toInstant())); | ||
| assertTrue(actual1.getInstant().isAfter(beforeStore.toInstant())); | ||
| assertTrue(!actual1.getInstant().isAfter(afterStore1.toInstant())); | ||
| assertTrue(!actual1.getInstant().isBefore(beforeStore.toInstant())); | ||
| }); | ||
| } | ||
|
|
||
|
|
@@ -331,8 +331,8 @@ public void testQueryUsingNameAndStartAndEndWithGTOperator() { | |
| HistoricItem actual1 = iterator.next(); | ||
| assertFalse(iterator.hasNext()); | ||
| assertStateEquals(getSecondItemState(), actual1.getState()); | ||
| assertTrue(actual1.getTimestamp().toInstant().isBefore(afterStore2.toInstant())); | ||
| assertTrue(actual1.getTimestamp().toInstant().isAfter(afterStore1.toInstant())); | ||
| assertTrue(!actual1.getTimestamp().toInstant().isAfter(afterStore2.toInstant())); | ||
| assertTrue(!actual1.getTimestamp().toInstant().isBefore(afterStore1.toInstant())); | ||
| }); | ||
| } | ||
|
|
||
|
|
@@ -368,8 +368,8 @@ public void testQueryUsingNameAndStartAndEndWithGTEOperator() { | |
| HistoricItem actual1 = iterator.next(); | ||
| assertFalse(iterator.hasNext()); | ||
| assertStateEquals(getSecondItemState(), actual1.getState()); | ||
| assertTrue(actual1.getTimestamp().toInstant().isBefore(afterStore2.toInstant())); | ||
| assertTrue(actual1.getTimestamp().toInstant().isAfter(afterStore1.toInstant())); | ||
| assertTrue(!actual1.getTimestamp().toInstant().isAfter(afterStore2.toInstant())); | ||
| assertTrue(!actual1.getTimestamp().toInstant().isBefore(afterStore1.toInstant())); | ||
| }); | ||
| } | ||
|
|
||
|
|
@@ -389,8 +389,8 @@ public void testQueryUsingNameAndStartAndEndFirst() { | |
| HistoricItem actual1 = iterator.next(); | ||
| assertFalse(iterator.hasNext()); | ||
| assertStateEquals(getFirstItemState(), actual1.getState()); | ||
| assertTrue(actual1.getTimestamp().toInstant().isBefore(afterStore1.toInstant())); | ||
| assertTrue(actual1.getTimestamp().toInstant().isAfter(beforeStore.toInstant())); | ||
| assertTrue(!actual1.getTimestamp().toInstant().isAfter(afterStore1.toInstant())); | ||
| assertTrue(!actual1.getTimestamp().toInstant().isBefore(beforeStore.toInstant())); | ||
| }); | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it guaranteed by the core that this timestamp is updated when persistence is called?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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