-
Notifications
You must be signed in to change notification settings - Fork 589
HDDS-3196 New PipelineManager interface to persist to RatisServer. #980
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
| } | ||
|
|
||
| public static PipelineManager newPipelineManager( | ||
| ConfigurationSource conf, SCMHAManager scmhaManager, |
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.
NIT: we can just pass RatisServer instead of SCMHAManager here.
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 may have more stuff in SCMHAManager than just SCMRatisServer for PipelineManager use cases. The idea here is to have a manager interface for SCM HA so that we won't worry about passing more things into PipelineManager like configs or other things. @xiaoyuyao
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.
In the future, we have to do isLeader check inside PipelineManager for which we might need SCMHAManager instance.
...dds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateManagerV2.java
Show resolved
Hide resolved
nandakumar131
left a comment
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.
Overall the patch looks good to me.
...dds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateManagerV2.java
Outdated
Show resolved
Hide resolved
...hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerV2Impl.java
Outdated
Show resolved
Hide resolved
a3e807c to
aac141d
Compare
nandakumar131
left a comment
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.
I'm ok with merging the changes and address the comments in the follow-up jiras.
...dds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateManagerV2.java
Outdated
Show resolved
Hide resolved
...server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateManagerV2Impl.java
Outdated
Show resolved
Hide resolved
...server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateManagerV2Impl.java
Outdated
Show resolved
Hide resolved
|
@xiaoyuyao, if you're ok with the changes I will go ahead and merge it. |
|
@timmylicheng thanks for the contribution. |
What changes were proposed in this pull request?
Construct new PipelineManager and PipelineStateManager interface for SCM.
(Please fill in changes proposed in this fix)
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3196
(Please create an issue in ASF JIRA before opening a pull request,
and you need to set the title of the pull request which starts with
the corresponding JIRA issue number. (e.g. HDDS-XXXX. Fix a typo in YYY.)
Please replace this section with the link to the Apache JIRA)
How was this patch tested?
TODO in https://issues.apache.org/jira/browse/HDDS-3679
(Please explain how this patch was tested. Ex: unit tests, manual tests)
(If this patch involves UI changes, please attach a screen-shot; otherwise, remove this)