smengcl commented on code in PR #11001:
URL: https://github.com/apache/ozone/pull/11001#discussion_r3770127940
##########
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:
The watched index is intentionally any value past the last log index. After
the leader and one follower are shut down and the pipeline is closed, no new
entries get committed, so `index + randomInt(0, 100) + 10` is always an
uncommitted index and the watch fails regardless of the offset, so the random
part does not affect determinism here. It also matches the existing pattern in
`testWatchForCommitForGroupMismatchException`, so I kept it for consistency.
##########
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 is the specific behavior the test needs to verify: that the client
actually attempts the watch at the requested replication level. Both levels
fail with the same exception types (NotReplicated, RetryFailure,
GroupMismatch), so asserting on the exception type cannot distinguish
MAJORITY_COMMITTED from ALL_COMMITTED. The log string comes from
`XceiverClientRatis` and is already asserted the same way in the sibling test
`test2WayCommitForTimeoutException`, so a wording change would surface in both.
Keeping it as is.
##########
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:
Good point. Added an early break once both the leader and a follower are
found, in a1d398c116b.
--
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]
