-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HDDS-1753. Datanode unable to find chunk while replication data using ratis. #1318
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bshashikant Thanks for working on this! The changes look good to me. I have couple of minor comments. There are a few checkstyle issues and test failures seem related.
...tainer-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java
Outdated
Show resolved
Hide resolved
...tainer-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java
Outdated
Show resolved
Hide resolved
...tainer-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java
Outdated
Show resolved
Hide resolved
...tainer-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java
Outdated
Show resolved
Hide resolved
...tainer-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java
Outdated
Show resolved
Hide resolved
...java/org/apache/hadoop/ozone/container/common/transport/server/ratis/XceiverServerRatis.java
Show resolved
Hide resolved
public long getMinReplicatedIndex(PipelineID pipelineID) throws IOException { | ||
Long minIndex = null; | ||
Iterator<RaftGroup> raftGroupIterator = getServer().getGroups().iterator(); | ||
while (raftGroupIterator.hasNext()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can directly use the getServer().getGroupInfo(..) api here and do not need the while loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed.
...tainer-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java
Outdated
Show resolved
Hide resolved
...ration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestDeleteWithSlowFollower.java
Show resolved
Hide resolved
...ration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestDeleteWithSlowFollower.java
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few checkstyle issues. TestBlockDeletion failure seems related.
XceiverServerRatis ratisServer = | ||
(XceiverServerRatis) ozoneContainer.getWriteChannel(); | ||
long minReplicatedIndex = ratisServer.getMinReplicatedIndex(PipelineID | ||
.valueOf(UUID.fromString(containerData.getOriginPipelineId()))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the pipeline does not exist, it should throw GroupMismatchException here. We can check what exception is thrown once. In that case we need to return true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in the latest patch.
Mockito.when(ozoneContainer.getContainerSet()) | ||
.thenReturn(containerSet); | ||
Mockito.when(ozoneContainer.getWriteChannel()) | ||
.thenReturn(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it is used in all the test classes, we can add the logic for mocking ozoneContainer in a TestUtil class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in the latest patch.
cluster.shutdownHddsDatanode(follower.getDatanodeDetails()); | ||
key.write(testData); | ||
key.close(); | ||
XceiverClientSpi xceiverClient = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please add a comment here that the container will be closed? Similarly a comment regarding deleting the key just so that we can separate the various sections of the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in the latest patch..
@bshashikant Thanks for updating the PR! The changes look good to me. +1. |
💔 -1 overall
This message was automatically generated. |
@bshashikant Thanks for working on the PR! I have merged it with trunk. |
No description provided.