Copilot commented on code in PR #11001:
URL: https://github.com/apache/ozone/pull/11001#discussion_r3765403737
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestWatchForCommit.java:
##########
@@ -255,34 +256,51 @@ public void
testWatchForCommitForRetryfailure(RaftProtos.ReplicationLevel watchT
xceiverClient.getPipeline()));
reply.getResponse().get();
long index = reply.getLogIndex();
- cluster.shutdownHddsDatanode(pipeline.getNodes().get(0));
- cluster.shutdownHddsDatanode(pipeline.getNodes().get(1));
+ // Shut down the Ratis leader and one follower so that no reachable
+ // leader remains to answer the watch with NotReplicatedException,
which
+ // would let an ALL_COMMITTED watch degrade to a normal (majority)
reply.
+ shutdownRatisLeaderAndOneFollower(pipeline);
// emulate closing pipeline when SCM detects DEAD datanodes
cluster.getStorageContainerManager()
.getPipelineManager().closePipeline(pipeline.getId());
- // again write data with more than max buffer limit. This wi
- // just watch for a log index which in not updated in the commitInfo
Map
- // as well as there is no logIndex generate in Ratis.
- // The basic idea here is just to test if its throws an exception.
+ // Watch for a log index which is neither present in the commitInfo map
+ // nor generated in Ratis. With no reachable leader this must fail.
+ // The basic idea here is just to test that it throws an exception.
ExecutionException e = assertThrows(ExecutionException.class,
() -> xceiverClient.watchForCommit(index +
RandomUtils.secure().randomInt(0, 100) + 10)
.get());
Review Comment:
This test still uses randomness to pick the watched index, which can make
failures harder to reproduce and can reintroduce nondeterminism. Consider using
a fixed offset (eg, `index + 10`) or a deterministic value derived from `index`
so the test remains fully reproducible while still ensuring it’s an uncommitted
index.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestWatchForCommit.java:
##########
@@ -255,34 +256,51 @@ public void
testWatchForCommitForRetryfailure(RaftProtos.ReplicationLevel watchT
xceiverClient.getPipeline()));
reply.getResponse().get();
long index = reply.getLogIndex();
- cluster.shutdownHddsDatanode(pipeline.getNodes().get(0));
- cluster.shutdownHddsDatanode(pipeline.getNodes().get(1));
+ // Shut down the Ratis leader and one follower so that no reachable
+ // leader remains to answer the watch with NotReplicatedException,
which
+ // would let an ALL_COMMITTED watch degrade to a normal (majority)
reply.
+ shutdownRatisLeaderAndOneFollower(pipeline);
// emulate closing pipeline when SCM detects DEAD datanodes
cluster.getStorageContainerManager()
.getPipelineManager().closePipeline(pipeline.getId());
- // again write data with more than max buffer limit. This wi
- // just watch for a log index which in not updated in the commitInfo
Map
- // as well as there is no logIndex generate in Ratis.
- // The basic idea here is just to test if its throws an exception.
+ // Watch for a log index which is neither present in the commitInfo map
+ // nor generated in Ratis. With no reachable leader this must fail.
+ // The basic idea here is just to test that it throws an exception.
ExecutionException e = assertThrows(ExecutionException.class,
() -> xceiverClient.watchForCommit(index +
RandomUtils.secure().randomInt(0, 100) + 10)
.get());
- // since the timeout value is quite long, the watch request will either
- // fail with NotReplicated exceptio, RetryFailureException or
- // RuntimeException
+ // The watch fails via retry/replication (or group mismatch) failure,
+ // not a bare timeout.
assertFalse(HddsClientUtils
.checkForException(e) instanceof TimeoutException);
- // client should not attempt to watch with
- // MAJORITY_COMMITTED replication level, except the grpc IO issue
- if (!logCapturer.getOutput().contains("Connection refused")) {
-
assertThat(e.getMessage()).doesNotContain("Watch-MAJORITY_COMMITTED");
- }
+ // The client attempts the watch at the requested replication level.
+ assertThat(logCapturer.getOutput()).contains(watchType + " way commit
failed");
Review Comment:
This assertion depends on an exact log string format (`watchType + \" way
commit failed\"`), which is brittle and can fail due to unrelated log message
wording changes. A more robust approach is to assert on a less specific
invariant (eg, that the output contains the replication level and a stable
failure token separately) or to assert directly on the exception/cause type(s)
that correspond to the requested replication-level failure path.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestWatchForCommit.java:
##########
@@ -255,34 +256,51 @@ public void
testWatchForCommitForRetryfailure(RaftProtos.ReplicationLevel watchT
xceiverClient.getPipeline()));
reply.getResponse().get();
long index = reply.getLogIndex();
- cluster.shutdownHddsDatanode(pipeline.getNodes().get(0));
- cluster.shutdownHddsDatanode(pipeline.getNodes().get(1));
+ // Shut down the Ratis leader and one follower so that no reachable
+ // leader remains to answer the watch with NotReplicatedException,
which
+ // would let an ALL_COMMITTED watch degrade to a normal (majority)
reply.
+ shutdownRatisLeaderAndOneFollower(pipeline);
// emulate closing pipeline when SCM detects DEAD datanodes
cluster.getStorageContainerManager()
.getPipelineManager().closePipeline(pipeline.getId());
- // again write data with more than max buffer limit. This wi
- // just watch for a log index which in not updated in the commitInfo
Map
- // as well as there is no logIndex generate in Ratis.
- // The basic idea here is just to test if its throws an exception.
+ // Watch for a log index which is neither present in the commitInfo map
+ // nor generated in Ratis. With no reachable leader this must fail.
+ // The basic idea here is just to test that it throws an exception.
ExecutionException e = assertThrows(ExecutionException.class,
() -> xceiverClient.watchForCommit(index +
RandomUtils.secure().randomInt(0, 100) + 10)
.get());
- // since the timeout value is quite long, the watch request will either
- // fail with NotReplicated exceptio, RetryFailureException or
- // RuntimeException
+ // The watch fails via retry/replication (or group mismatch) failure,
+ // not a bare timeout.
assertFalse(HddsClientUtils
.checkForException(e) instanceof TimeoutException);
- // client should not attempt to watch with
- // MAJORITY_COMMITTED replication level, except the grpc IO issue
- if (!logCapturer.getOutput().contains("Connection refused")) {
-
assertThat(e.getMessage()).doesNotContain("Watch-MAJORITY_COMMITTED");
- }
+ // The client attempts the watch at the requested replication level.
+ assertThat(logCapturer.getOutput()).contains(watchType + " way commit
failed");
} finally {
clientManager.releaseClient(xceiverClient, false);
}
}
}
+ private void shutdownRatisLeaderAndOneFollower(Pipeline pipeline) throws
Exception {
+ DatanodeDetails leader = null;
+ DatanodeDetails follower = null;
+ for (HddsDatanodeService dn : cluster.getHddsDatanodes()) {
+ DatanodeDetails details = dn.getDatanodeDetails();
+ if (!pipeline.getNodes().contains(details)) {
+ continue;
+ }
+ if (RatisTestHelper.isRatisLeader(dn, pipeline)) {
+ leader = details;
+ } else if (follower == null && RatisTestHelper.isRatisFollower(dn,
pipeline)) {
+ follower = details;
+ }
+ }
Review Comment:
Once both `leader` and `follower` are found, the loop can break early. This
avoids extra `isRatisLeader/isRatisFollower` calls and makes the intent clearer
(find exactly one leader and one follower, then stop scanning).
--
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]
