HDDS-1610. applyTransaction failure should not be lost on restart. #1226
Conversation
|
💔 -1 overall
This message was automatically generated. |
| if (r.getResult() != ContainerProtos.Result.SUCCESS) { | ||
| StorageContainerException sce = | ||
| new StorageContainerException(r.getMessage(), r.getResult()); | ||
| LOG.error(gid + ": ApplyTransaction failed: cmd " + r.getCmdType() |
There was a problem hiding this comment.
Should we add containerID in this error log?
There was a problem hiding this comment.
Container Id will be present in the Response Message. Will add that to the logger output.
There was a problem hiding this comment.
lets convert this to parameterized logging.
| String msg = | ||
| "Ratis Transaction failure in datanode" + dnId + " with role " + role | ||
| + " Triggering pipeline close action."; | ||
| triggerPipelineClose(groupId, msg, ClosePipelineInfo.Reason.PIPELINE_FAILED, |
There was a problem hiding this comment.
Do we need a new reason code here? Pipeline Failed is getting overloaded.
There was a problem hiding this comment.
I think, the msg will differentiate what was the cause of the error. The reason code is just for SCM to take action of closing the pipeline. I don't think possibly SCM needs to differentiate its behaviour depending on why the pipelien failed.
If required, we can add it in a separate jira as it needs to change for other reasons of pipeline failure.
There was a problem hiding this comment.
Lets add a new failure reason here as @supratimdeka is suggesting. This will help in identifying difference failure via metrics in SCM.
| client.sendCommand(request.build()); | ||
| Assert.fail("Expected exception not thrown"); | ||
| } catch (IOException e) { | ||
| } |
There was a problem hiding this comment.
Perhaps a log message here to say that the test caught an IOException as expected by the test case.
There was a problem hiding this comment.
Will address in the next patch..
| XceiverServerRatis raftServer = (XceiverServerRatis) | ||
| cluster.getHddsDatanodes().get(0).getDatanodeStateMachine() | ||
| .getContainer().getWriteChannel(); | ||
| Assert.assertTrue(raftServer.isClosed()); |
There was a problem hiding this comment.
one key goal of HDDS-1610 is to ensure that no snapshot can be taken after a log apply failure.
Should the unit test include this assertion? Perhaps by setting the autoTriggerThreshold in Ratis to take a snapshot after every applyLog.
There was a problem hiding this comment.
will address in the next patch.
There was a problem hiding this comment.
Thanks for working on this @bshashikant.
This patch does not fix the problem in HDDS-1610, as even after a applyTransactionFailure,
the raftServer shutdown will trigger a snapshot create as part of shutdown.
I also feel that the metadata around failure should be maintained inside the ContainerStateMachine as the shutdown behavior of the ratis server can most probably change in the future.
| updateLastApplied(); | ||
| }).whenComplete((r, t) -> applyTransactionSemaphore.release()); | ||
| return future; | ||
| applyTransactionSemaphore.release(); |
There was a problem hiding this comment.
This should go inside a finally, to make sure the semaphore is released always.
| key.write("ratis".getBytes()); | ||
|
|
||
| //get the name of a valid container | ||
| OmKeyArgs keyArgs = new OmKeyArgs.Builder().setVolumeName(volumeName). |
| request.setContainerID(containerID); | ||
| request.setCloseContainer( | ||
| ContainerProtos.CloseContainerRequestProto.getDefaultInstance()); | ||
| // close container transaction will fail over Ratis and will cause the raft |
| client.sendCommand(request.build()); | ||
| Assert.fail("Expected exception not thrown"); | ||
| } catch (IOException e) { | ||
| } |
There was a problem hiding this comment.
What exception are we expecting here ?
| Pipeline pipeline = cluster.getStorageContainerLocationClient() | ||
| .getContainerWithPipeline(containerID).getPipeline(); | ||
| XceiverClientSpi client = xceiverClientManager.acquireClient(pipeline); | ||
| ContainerProtos.ContainerCommandRequestProto.Builder request = |
There was a problem hiding this comment.
How about we append some more data to the key and flush again ? in place of a close ?
There was a problem hiding this comment.
The idea is to execute a transaction on the same container. If we write more data , it can potentially go a new container altogether.
| @@ -630,6 +640,10 @@ void handleInstallSnapshotFromLeader(RaftGroupId groupId, | |||
| handlePipelineFailure(groupId, roleInfoProto); | |||
| } | |||
|
|
|||
| @VisibleForTesting | |||
| public boolean isClosed() { | |||
There was a problem hiding this comment.
lets remove this. and this can be replaced with
raftServer.getServer().getLifeCycleState().isClosingOrClosed()
| + " Triggering pipeline close action."; | ||
| triggerPipelineClose(groupId, msg, ClosePipelineInfo.Reason.PIPELINE_FAILED, | ||
| false); | ||
| stop(); |
There was a problem hiding this comment.
We do not necessarily need to stop the raftServer here, for the other container's we can still keep on applying the transaction
There was a problem hiding this comment.
As far as i know from previous discussions , the decision was to not take any other transactions on this pipeline at all and kill the RaftServerImpl instance. Any deviation from that conclusion?
| .supplyAsync(() -> runCommand(requestProto, builder.build()), | ||
| CompletableFuture<ContainerCommandResponseProto> future = | ||
| CompletableFuture.supplyAsync( | ||
| () -> runCommandGetResponse(requestProto, builder.build()), |
There was a problem hiding this comment.
lets rename runCommandGetResponse and remove runCommand as all the existing caller of the earlier function runCommand can be removed.
| if (r.getResult() != ContainerProtos.Result.SUCCESS) { | ||
| StorageContainerException sce = | ||
| new StorageContainerException(r.getMessage(), r.getResult()); | ||
| LOG.error(gid + ": ApplyTransaction failed: cmd " + r.getCmdType() |
There was a problem hiding this comment.
lets convert this to parameterized logging.
| requestProto.getWriteChunk().getChunkData().getLen()); | ||
| LOG.debug(gid + ": ApplyTransaction completed: cmd " + r.getCmdType() |
There was a problem hiding this comment.
if this is a success, then "" Error message: " + r.getMessage()" will not be the right thing to print here.
|
Thanks @mukul1987 . In ratis, as far as my understanding goes, before taking a snapshot we wait for all the pending applyTrannsaction futures to complete and since now with the patch, the applyTransaction exception is being propagated to Ratis, ideally snapshot creation will fail in Ratis. I will add a test case to verify the same. |
|
💔 -1 overall
This message was automatically generated. |
| updateLastApplied(); | ||
| }).whenComplete((r, t) -> applyTransactionSemaphore.release()); | ||
| return future; | ||
| applyTransactionSemaphore.release(); |
There was a problem hiding this comment.
I think we should keep the whenComplete() stage at the end.
releasing the semaphore from a whenComplete() stage guarantees that the semaphore will be released even if the processing inside thenApply() stage hits an exception. This seems to me to be a good practice.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Thanks for working on this @bshashikant. The patch looks good to me. Some minor comments.
| if (!isStateMachineHealthy.get()) { | ||
| String msg = | ||
| "Failed to take snapshot " + " for " + gid + " as the stateMachine" | ||
| + " is unhealthy. The last applied index is at " + ti; |
There was a problem hiding this comment.
Addressed in the latest patch.
| // before any further snapshot is taken , the exception will be | ||
| // caught in stateMachineUpdater in Ratis and ratis server will | ||
| // shutdown. | ||
| applyTransactionFuture.completeExceptionally(sce); |
There was a problem hiding this comment.
lets move the ratisServer.handleApplyTransactionFailure(gid, trx.getServerRole()); as the last line in the if block.
There was a problem hiding this comment.
Addressed in the latest patch.
|
💔 -1 overall
This message was automatically generated. |
|
/retest |
|
💔 -1 overall
This message was automatically generated. |
|
The test failures are not related to the patch. |
|
Thanks for working on this @bshashikant. +1 the patch looks good to me. The test failures do not look related. |
|
Thanks @mukul1987 and @supratimdeka for the review. I have committed the change to trunk. |
| * during key.close() because the first container is UNHEALTHY by that time | ||
| */ | ||
| Assert.assertTrue("Expect Key to be stored in 2 separate containers", | ||
| keyDetails.getOzoneKeyLocations().size() == 2); | ||
| } |
There was a problem hiding this comment.
I am creating a tool to aid developers by partially automating unit testing.
Could you speak to the validity of these generated assert statement for the given test method?
org.junit.Assert.assertEquals( OmKeyLocationInfo.getHddsDatanodes(), groupOutputStream.getHddsDatanodes( groupOutputStream ));
Thank you.
| @@ -270,4 +269,83 @@ public void testUnhealthyContainer() throws Exception { | |||
| Assert.assertEquals(ContainerProtos.Result.CONTAINER_UNHEALTHY, | |||
| dispatcher.dispatch(request.build(), null).getResult()); | |||
| } | |||
There was a problem hiding this comment.
Could you speak to the validity of these generated assert statement for the above test method?
org.junit.Assert.assertTrue( ONE.readContainerFile( node ) );
org.junit.Assert.assertTrue( ONE.readContainerFile( node.toString() ));
org.junit.Assert.assertTrue( ONE.readContainerFile( node.toString(), KeyOutputStream ))
Thank you.
| // Make sure the latest snapshot is same as the previous one | ||
| FileInfo latestSnapshot = storage.findLatestSnapshot().getFile(); | ||
| Assert.assertTrue(snapshot.getPath().equals(latestSnapshot.getPath())); | ||
| } | ||
| } |
There was a problem hiding this comment.
Could you speak to the validity of these generated assert statement for the above test method?
org.junit.Assert.assertTrue( name, volumeName.isValid() )
Thank you.
…ntributed by Shashikant Banerjee(#1226).
…ntributed by Shashikant Banerjee(apache#1226).
…ntributed by Shashikant Banerjee(apache#1226).
No description provided.