chihsuan commented on code in PR #11227:
URL: https://github.com/apache/ozone/pull/11227#discussion_r4026148188
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/storage/TestCommitWatcher.java:
##########
@@ -223,6 +238,15 @@ public void testReleaseBuffersOnException() throws
Exception {
try (XceiverClientSpi xceiverClient = mgr.acquireClient(pipeline)) {
assertEquals(1, xceiverClient.getRefcount());
XceiverClientRatis ratisClient =
assertInstanceOf(XceiverClientRatis.class, xceiverClient);
+ // Ensure the freshly-allocated pipeline has an elected Ratis leader
+ // before the async write burst: register the Ratis group on the
+ // pipeline datanodes and commit a CreateContainer synchronously.
+ // Otherwise the first write races leader election and can fail with
+ // NotLeaderException -> RaftRetryFailureException ->
AlreadyClosedException.
+ OzoneTestHelper.createPipelineOnDatanode(pipeline, cluster);
+ ratisClient.sendCommandAsync(
+ ContainerTestHelper.getCreateContainerRequest(containerId,
pipeline))
+ .getResponse().get();
Review Comment:
I wonder if leader election is really the cause here. In the DN logs from my
run, the second `WriteChunk` failed with `CHUNK_FILE_INCONSISTENCY` after the
first `PutBlock` closed the block file. The container was then marked
UNHEALTHY, followed by pipeline closure. Would using a different `blockID` per
iteration avoid this?
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/storage/TestCommitWatcher.java:
##########
@@ -128,6 +129,11 @@ public void init() throws Exception {
.setNumDatanodes(5)
.build();
cluster.waitForClusterToBeReady();
+ // Wait for the RATIS THREE pipeline to reach OPEN state before any writes.
+ // A pipeline only opens once it is healthy, which requires an elected
Ratis
+ // leader; otherwise the first write can race leader election and fail with
+ // NotLeaderException -> RaftRetryFailureException ->
AlreadyClosedException.
+ cluster.waitForPipelineTobeReady(HddsProtos.ReplicationFactor.THREE,
60000);
Review Comment:
Do we still need this wait? I noticed `allocateContainer` only picks
pipelines already in OPEN state, and OPEN already implies a reported leader, so
this may not add any guarantee.
https://github.com/apache/ozone/blob/f9f61847685b15e7aa743079f0ef146053a93d9b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java#L182-L186
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/storage/TestCommitWatcher.java:
##########
@@ -157,6 +163,15 @@ public void testReleaseBuffers() throws Exception {
try (XceiverClientSpi xceiverClient = mgr.acquireClient(pipeline)) {
assertEquals(1, xceiverClient.getRefcount());
XceiverClientRatis ratisClient =
assertInstanceOf(XceiverClientRatis.class, xceiverClient);
+ // Ensure the freshly-allocated pipeline has an elected Ratis leader
Review Comment:
nit: Could we trim this comment and keep the explanation in one place? The
same explanation appears here, at line 241, and in `init()`, and the exception
chain is already in the PR description.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/storage/TestCommitWatcher.java:
##########
@@ -157,6 +163,15 @@ public void testReleaseBuffers() throws Exception {
try (XceiverClientSpi xceiverClient = mgr.acquireClient(pipeline)) {
assertEquals(1, xceiverClient.getRefcount());
XceiverClientRatis ratisClient =
assertInstanceOf(XceiverClientRatis.class, xceiverClient);
+ // Ensure the freshly-allocated pipeline has an elected Ratis leader
+ // before the async write burst: register the Ratis group on the
+ // pipeline datanodes and commit a CreateContainer synchronously.
+ // Otherwise the first write races leader election and can fail with
+ // NotLeaderException -> RaftRetryFailureException ->
AlreadyClosedException.
+ OzoneTestHelper.createPipelineOnDatanode(pipeline, cluster);
Review Comment:
nit: Could we drop `createPipelineOnDatanode` here? The groups should
already exist for an OPEN pipeline, so these calls normally just hit
duplicate-group errors that the helper swallows. Same for line 246.
--
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]
