ivandika3 commented on code in PR #10959:
URL: https://github.com/apache/ozone/pull/10959#discussion_r3733758774
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -288,6 +288,10 @@ public void testInstallSnapshotWithClientWrite() throws
Exception {
});
List<String> newKeys = writeFuture.get();
+ // A write is acked once queued in the double buffer, but the applied index
+ // advances only after the buffer commits. Flush so it covers all newKeys.
+ leaderOM.awaitDoubleBufferFlush();
Review Comment:
Is this the same workaround of https://github.com/apache/ozone/pull/10748
(i.e. flush the double buffer so that the key is visible)? If that's so, I
don't agree with this as mentioned in that ticket since this means the OM
linearizability can be violated. We should not need to wait for double buffer
flush for the key to be visible. We should fix the root cause, not just trying
to pass the tests. Please correct me if I'm wrong about the OM consistency
guarantee since it's never properly expounded or proven by the original author.
The only time manually test triggered flush is valid is for OMResponse unit
test, not in integration tests. I saw a lot of `awaitDoubleBufferFlush`(or
worse, sleep) in integration test which is worrying.
But if it's because of the appliedIndex check
```java
// The recently started OM should be lagging behind the leader OM.
// Wait & for follower to update transactions to leader snapshot index.
// Timeout error if follower does not load update within 3s
GenericTestUtils.waitFor(() -> {
return
followerOM.getOmRatisServer().getLastAppliedTermIndex().getIndex()
>= leaderOMSnapshotIndex - 1;
}, 100, 30_000);
```
I might be fine with it. In that case, please move this after the key
visibility check.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -288,6 +288,10 @@ public void testInstallSnapshotWithClientWrite() throws
Exception {
});
List<String> newKeys = writeFuture.get();
+ // A write is acked once queued in the double buffer, but the applied index
+ // advances only after the buffer commits. Flush so it covers all newKeys.
+ leaderOM.awaitDoubleBufferFlush();
Review Comment:
Is this the same workaround of https://github.com/apache/ozone/pull/10748
(i.e. flush the double buffer so that the key is visible)? If that's so, I
don't agree with this as mentioned in that ticket since this means the OM
linearizability can be violated. We should not need to wait for double buffer
flush for the key to be visible. We should fix the root cause, not just trying
to pass the tests. Please correct me if I'm wrong about the OM consistency
guarantee since it's never properly expounded or proven by the original author.
The only time manually test triggered flush is valid is for OMResponse unit
test, not in integration tests. I saw a lot of `awaitDoubleBufferFlush`(or
worse, sleep workaround) in integration test which is worrying.
But if it's because of the appliedIndex check
```java
// The recently started OM should be lagging behind the leader OM.
// Wait & for follower to update transactions to leader snapshot index.
// Timeout error if follower does not load update within 3s
GenericTestUtils.waitFor(() -> {
return
followerOM.getOmRatisServer().getLastAppliedTermIndex().getIndex()
>= leaderOMSnapshotIndex - 1;
}, 100, 30_000);
```
I might be fine with it. In that case, please move this after the key
visibility check.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
