HDDS-3186. Introduce generic SCMRatisRequest and SCMRatisResponse. #959
Conversation
|
/pending "Not ready for review" |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
nandakumar131
changed the title
timmylicheng
left a comment
timmylicheng
left a comment
There was a problem hiding this comment.
Good work on the @replication interface!
We just need to merge two sets of SCMRatisServer and SCMStateMachine into one and we can worry about other TODOs in later commits.
| import java.util.Collections; | ||
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| public class RatisUtil { |
There was a problem hiding this comment.
can we put this into SCMHAUtils?
There was a problem hiding this comment.
I have moved this to ReflectionUtil class which can be used for all the utility methods related to reflection. I will update the PR soon.
I agree that we should use the existing SCMHAUtil instead of creating new RatisUtil
There was a problem hiding this comment.
https://issues.apache.org/jira/browse/HDDS-3660 Track it here
There was a problem hiding this comment.
I feel it's ok to have ReflectionUtil, RatisUtil, and SCMHAUtil as they serve a different purpose.
| import org.apache.ratis.protocol.RaftPeerId; | ||
| import org.apache.ratis.server.RaftServer; | ||
|
|
||
| public class SCMRatisServer { |
There was a problem hiding this comment.
I've merged SCMRatisServer and SCMStateMachine into one between /ha and /ratis in timmylicheng#1. We can use /ha as your did here, but we need to combine all methods into one.
There was a problem hiding this comment.
I don't think we need to copy all the code/methods from OzoneManagerHA implementation. We can add things to SCMHA related classes whenever required.
It's better not to have code that is not used or needed.
Let's add/update the SCMHA code when needed.
There was a problem hiding this comment.
How about Snapshot in SCMStateMachine?
There was a problem hiding this comment.
Track the issue in https://issues.apache.org/jira/browse/HDDS-3661.
This is to split the work.
There was a problem hiding this comment.
Let's configure/enable Ratis snapshot after we have some design plan on how to implement the snapshot.
|
/ready |
github-actions
bot
dismissed
their stale review
Blocking review request is removed.
| .setReplicationFactor(pipeline.getFactor()) | ||
| .setReplicationType(pipeline.getType()) | ||
| .build(); | ||
| containerStateManager.addContainer(containerInfo); |
There was a problem hiding this comment.
Do we need to addContainerToDB here?
There was a problem hiding this comment.
It is handled inside ContainerStateManager.
|
+1 merge in dev branch and pending on following JIRAs to resolve remaining works. |
|
@timmylicheng thanks for the merge! |
2 participants
What changes were proposed in this pull request?
This PR will introduce generic
SCMRatisRequestandSCMRatisResponsewhich will be used by all the Ratis operations inside SCM. We also have a genericStateMachinewhich will dispatch the request to registered handlers.What is the link to the Apache JIRA
HDDS-3186
How was this patch tested?
This patch is not tested properly.
The following jiras are created for adding unit tests to test the changes introduced in this PR
HDDS-3650, HDDS-3651 and HDDS-3652