rpuch commented on code in PR #2319:
URL: https://github.com/apache/ignite-3/pull/2319#discussion_r1263656866
##########
modules/core/src/main/java/org/apache/ignite/internal/causality/IncrementalVersionedValue.java:
##########
@@ -63,7 +63,14 @@ public class IncrementalVersionedValue<T> implements
VersionedValue<T> {
/**
* This registry chains two versioned values. The value, that uses this
registry in the constructor, will be completed strictly after
- * the value, passed into this method.
+ * the value, passed into this method, meaning that {@code
resultVv.get(token).isDone();} will always imply imply
Review Comment:
```suggestion
* the value, passed into this method, meaning that {@code
resultVv.get(token).isDone();} will always imply
```
##########
modules/core/src/test/java/org/apache/ignite/internal/causality/IncrementalVersionedValueTest.java:
##########
@@ -341,6 +344,51 @@ void testCompleteMultipleFutures() {
assertThat(future3, willBe(2));
}
+ /**
+ * Tests that {@link
IncrementalVersionedValue#dependingOn(IncrementalVersionedValue)} provides
causality between 2 different values.
+ */
+ @RepeatedTest(100)
+ public void testDependingOn() {
+ var vv0 = new IncrementalVersionedValue<>(register, () -> 1);
+
+ var vv1 = new IncrementalVersionedValue<>(dependingOn(vv0), () -> 1);
+
+ int token = 1;
+
+ vv0.update(token, (i, e) -> completedFuture(i + 1));
+
+ vv1.update(token, (i, e) -> completedFuture(i + 1));
+
+ register.moveRevision(token);
Review Comment:
Maybe I'm looking in the wrong direction, but I don't see any concurrency
here (no async calls), so everything will be executed in the same thread and
the order of execution will be deterministic. Would it make sense to make the
test more interesting by using something like `supplyAsync()` instead of
`completedFuture()`?
##########
modules/core/src/test/java/org/apache/ignite/internal/causality/IncrementalVersionedValueTest.java:
##########
@@ -341,6 +344,51 @@ void testCompleteMultipleFutures() {
assertThat(future3, willBe(2));
}
+ /**
+ * Tests that {@link
IncrementalVersionedValue#dependingOn(IncrementalVersionedValue)} provides
causality between 2 different values.
+ */
+ @RepeatedTest(100)
+ public void testDependingOn() {
+ var vv0 = new IncrementalVersionedValue<>(register, () -> 1);
+
+ var vv1 = new IncrementalVersionedValue<>(dependingOn(vv0), () -> 1);
+
+ int token = 1;
+
+ vv0.update(token, (i, e) -> completedFuture(i + 1));
+
+ vv1.update(token, (i, e) -> completedFuture(i + 1));
+
+ register.moveRevision(token);
+
+ // Will always complete in time.
+ vv1.get(token).join();
+
+ assertTrue(vv0.get(token).isDone());
+ }
+
+ /**
+ * Tests that {@link IncrementalVersionedValue#update(long, BiFunction)}
closure is called immediately when underlying value is
+ * accessible, i.e. when there were no other updates.
+ */
+ @Test
+ public void testImmediateUpdate() {
Review Comment:
Should this be tested? Is this property a desired property important for the
contract of `IncrementalVersionedValue`, or it's just a coincidence that the
current implementation behaves like this?
##########
modules/core/src/test/java/org/apache/ignite/internal/causality/IncrementalVersionedValueTest.java:
##########
@@ -341,6 +344,51 @@ void testCompleteMultipleFutures() {
assertThat(future3, willBe(2));
}
+ /**
+ * Tests that {@link
IncrementalVersionedValue#dependingOn(IncrementalVersionedValue)} provides
causality between 2 different values.
+ */
+ @RepeatedTest(100)
+ public void testDependingOn() {
+ var vv0 = new IncrementalVersionedValue<>(register, () -> 1);
+
+ var vv1 = new IncrementalVersionedValue<>(dependingOn(vv0), () -> 1);
+
+ int token = 1;
+
+ vv0.update(token, (i, e) -> completedFuture(i + 1));
+
+ vv1.update(token, (i, e) -> completedFuture(i + 1));
+
+ register.moveRevision(token);
+
+ // Will always complete in time.
+ vv1.get(token).join();
Review Comment:
How about using `assertThat(..., willCompleteSuccessfully())` just in case
there is a bug preventing the completion?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
