You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2020/07/10 08:46:45 UTC
[GitHub] [hadoop-ozone] timmylicheng opened a new pull request #1191: HDDS-3837 Add isLeader check in SCMHAManager.
timmylicheng opened a new pull request #1191:
URL: https://github.com/apache/hadoop-ozone/pull/1191
## What changes were proposed in this pull request?
Add isLeader check in SCMHAManager.
(Please fill in changes proposed in this fix)
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3837
(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?
UT
(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)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1191: HDDS-3837 Add isLeader check in SCMHAManager.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1191:
URL: https://github.com/apache/hadoop-ozone/pull/1191#discussion_r463349579
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisServerImpl.java
##########
@@ -110,4 +111,18 @@ public void stop() throws IOException {
server.close();
}
+ @Override
+ public RaftServer getServer() {
+ return server;
+ }
+
+ @Override
+ public RaftGroupId getRaftGroupId() {
+ return raftGroupId;
+ }
+
+ @Override
+ public List<RaftPeer> getRaftPeers() {
+ return Collections.singletonList(new RaftPeer(raftPeerId));
Review comment:
why ScmRatiSserver only have one entry in the raft peers, is this something we plan to implement in future?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] timmylicheng closed pull request #1191: HDDS-3837 Add isLeader check in SCMHAManager.
Posted by GitBox <gi...@apache.org>.
timmylicheng closed pull request #1191:
URL: https://github.com/apache/hadoop-ozone/pull/1191
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] timmylicheng commented on a change in pull request #1191: HDDS-3837 Add isLeader check in SCMHAManager.
Posted by GitBox <gi...@apache.org>.
timmylicheng commented on a change in pull request #1191:
URL: https://github.com/apache/hadoop-ozone/pull/1191#discussion_r454862522
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAManagerImpl.java
##########
@@ -56,7 +69,28 @@ public void start() throws IOException {
*/
@Override
public boolean isLeader() {
- return isLeader;
+ if (!SCMHAUtils.isSCMHAEnabled(conf)) {
+ // When SCM HA is not enabled, the current SCM is always the leader.
+ return true;
+ }
Review comment:
https://issues.apache.org/jira/browse/HDDS-3962
Created a JIRA to track this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1191: HDDS-3837 Add isLeader check in SCMHAManager.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1191:
URL: https://github.com/apache/hadoop-ozone/pull/1191#discussion_r463350598
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerV2Impl.java
##########
@@ -136,6 +139,9 @@ public static PipelineManagerV2Impl newPipelineManager(
@Override
public Pipeline createPipeline(ReplicationType type,
ReplicationFactor factor) throws IOException {
+ if (!scmhaManager.isLeader()) {
Review comment:
NIT: Can we add a helper function like checkLeader() to dedup the code?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1191: HDDS-3837 Add isLeader check in SCMHAManager.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1191:
URL: https://github.com/apache/hadoop-ozone/pull/1191#discussion_r463351164
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerV2Impl.java
##########
@@ -431,15 +464,21 @@ public void scrubPipeline(ReplicationType type, ReplicationFactor factor)
* Schedules a fixed interval job to create pipelines.
*/
@Override
- public void startPipelineCreator() {
+ public void startPipelineCreator() throws NotLeaderException {
+ if (!scmhaManager.isLeader()) {
Review comment:
when scm leader changes, how to ensure the creator and scrubber threads are started on the new leader and stopped on the old leader?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1191: HDDS-3837 Add isLeader check in SCMHAManager.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1191:
URL: https://github.com/apache/hadoop-ozone/pull/1191#discussion_r463348101
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAManagerImpl.java
##########
@@ -67,6 +101,44 @@ public SCMRatisServer getRatisServer() {
return ratisServer;
}
+ private RaftPeerId getPeerIdFromRoleInfo(RaftServerImpl serverImpl) {
+ if (serverImpl.isLeader()) {
+ return RaftPeerId.getRaftPeerId(
+ serverImpl.getRoleInfoProto().getLeaderInfo().toString());
+ } else if (serverImpl.isFollower()) {
+ return RaftPeerId.valueOf(
+ serverImpl.getRoleInfoProto().getFollowerInfo()
+ .getLeaderInfo().getId().getId());
+ } else {
+ return null;
+ }
+ }
+
+ @Override
+ public RaftPeer getSuggestedLeader() {
+ RaftServer server = ratisServer.getServer();
+ Preconditions.checkState(server instanceof RaftServerProxy);
+ RaftServerImpl serverImpl = null;
+ try {
+ // SCM only has one raft group.
+ serverImpl = ((RaftServerProxy) server)
+ .getImpl(ratisServer.getRaftGroupId());
+ } catch (IOException ioe) {
+ LOG.error("Fail to get RaftServer impl and therefore it's not clear " +
+ "whether it's leader. ", ioe);
+ } finally {
+ if (serverImpl != null) {
Review comment:
Same as above, can we move this to the try block?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1191: HDDS-3837 Add isLeader check in SCMHAManager.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1191:
URL: https://github.com/apache/hadoop-ozone/pull/1191#discussion_r463350043
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NonHealthyToHealthyNodeHandler.java
##########
@@ -42,6 +47,11 @@ public NonHealthyToHealthyNodeHandler(
@Override
public void onMessage(DatanodeDetails datanodeDetails,
EventPublisher publisher) {
- pipelineManager.triggerPipelineCreation();
+ try {
+ pipelineManager.triggerPipelineCreation();
+ } catch (NotLeaderException ex) {
+ LOG.warn("Not the current leader SCM and cannot start pipeline" +
Review comment:
Same as above.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] timmylicheng commented on a change in pull request #1191: HDDS-3837 Add isLeader check in SCMHAManager.
Posted by GitBox <gi...@apache.org>.
timmylicheng commented on a change in pull request #1191:
URL: https://github.com/apache/hadoop-ozone/pull/1191#discussion_r454199946
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerV2Impl.java
##########
@@ -327,6 +349,9 @@ public void openPipeline(PipelineID pipelineId) throws IOException {
* @throws IOException
*/
protected void removePipeline(Pipeline pipeline) throws IOException {
+ if (!scmhaManager.isLeader()) {
Review comment:
If we want to refactor the code, I would prefer annotation just like @Replicate in which we would create something like CheckLeaderInvocationHandler extends InvocationHandler and do isLeader check there.
It would be convenient in interface. However, We also use isLeader check in some local methods instead of override methods. Let's keep it this fashion for now and we see if we can polish this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] GlenGeng commented on a change in pull request #1191: HDDS-3837 Add isLeader check in SCMHAManager.
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #1191:
URL: https://github.com/apache/hadoop-ozone/pull/1191#discussion_r454161936
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerV2Impl.java
##########
@@ -327,6 +349,9 @@ public void openPipeline(PipelineID pipelineId) throws IOException {
* @throws IOException
*/
protected void removePipeline(Pipeline pipeline) throws IOException {
+ if (!scmhaManager.isLeader()) {
Review comment:
Would it be cleaner by using `InvocationHander` to inject these `isLeader()` check ?
```
StateManager pipelineStateManager = (StateManager) Proxy.newProxyInstance(
null,
new Class<?>[] {StateManager.class},
(proxy, method, args) -> {
if (method.getName().equals("xxx")) {
// do is leader check;
// throw exception if needed;
}
return method.invoke(args);
});
```
You may use `hashmap` or `annotation` to speed up the method name check.
Just a recommendation.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] timmylicheng commented on a change in pull request #1191: HDDS-3837 Add isLeader check in SCMHAManager.
Posted by GitBox <gi...@apache.org>.
timmylicheng commented on a change in pull request #1191:
URL: https://github.com/apache/hadoop-ozone/pull/1191#discussion_r464418677
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerV2Impl.java
##########
@@ -431,15 +464,21 @@ public void scrubPipeline(ReplicationType type, ReplicationFactor factor)
* Schedules a fixed interval job to create pipelines.
*/
@Override
- public void startPipelineCreator() {
+ public void startPipelineCreator() throws NotLeaderException {
+ if (!scmhaManager.isLeader()) {
Review comment:
My thought:
1. Every SCM starts a pipeline creator thread. So I remove checkLeader in startPipelineCreator
2. For triggerPipelineCreator, if we decide to let follower go thru safemode once it steps up to be leader, we can have checkLeader there as well.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerV2Impl.java
##########
@@ -136,6 +139,9 @@ public static PipelineManagerV2Impl newPipelineManager(
@Override
public Pipeline createPipeline(ReplicationType type,
ReplicationFactor factor) throws IOException {
+ if (!scmhaManager.isLeader()) {
Review comment:
Updated.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] GlenGeng commented on a change in pull request #1191: HDDS-3837 Add isLeader check in SCMHAManager.
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #1191:
URL: https://github.com/apache/hadoop-ozone/pull/1191#discussion_r454752901
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAManagerImpl.java
##########
@@ -56,7 +69,28 @@ public void start() throws IOException {
*/
@Override
public boolean isLeader() {
- return isLeader;
+ if (!SCMHAUtils.isSCMHAEnabled(conf)) {
+ // When SCM HA is not enabled, the current SCM is always the leader.
+ return true;
+ }
Review comment:
Add a todo please
```
// will change from RaftServerImpl:::isLeader() to RaftServerImpl::getRoleInfoProto()
// after RATIS-1001 is merged.
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] timmylicheng closed pull request #1191: HDDS-3837 Add isLeader check in SCMHAManager.
Posted by GitBox <gi...@apache.org>.
timmylicheng closed pull request #1191:
URL: https://github.com/apache/hadoop-ozone/pull/1191
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] timmylicheng commented on a change in pull request #1191: HDDS-3837 Add isLeader check in SCMHAManager.
Posted by GitBox <gi...@apache.org>.
timmylicheng commented on a change in pull request #1191:
URL: https://github.com/apache/hadoop-ozone/pull/1191#discussion_r464418829
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NonHealthyToHealthyNodeHandler.java
##########
@@ -42,6 +47,11 @@ public NonHealthyToHealthyNodeHandler(
@Override
public void onMessage(DatanodeDetails datanodeDetails,
EventPublisher publisher) {
- pipelineManager.triggerPipelineCreation();
+ try {
+ pipelineManager.triggerPipelineCreation();
+ } catch (NotLeaderException ex) {
+ LOG.warn("Not the current leader SCM and cannot start pipeline" +
Review comment:
Updated
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NewNodeHandler.java
##########
@@ -41,6 +46,11 @@ public NewNodeHandler(PipelineManager pipelineManager,
@Override
public void onMessage(DatanodeDetails datanodeDetails,
EventPublisher publisher) {
- pipelineManager.triggerPipelineCreation();
+ try {
+ pipelineManager.triggerPipelineCreation();
+ } catch (NotLeaderException ex) {
+ LOG.warn("Not the current leader SCM and cannot start pipeline" +
Review comment:
Updated
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao merged pull request #1191: HDDS-3837 Add isLeader check in SCMHAManager.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao merged pull request #1191:
URL: https://github.com/apache/hadoop-ozone/pull/1191
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] timmylicheng commented on a change in pull request #1191: HDDS-3837 Add isLeader check in SCMHAManager.
Posted by GitBox <gi...@apache.org>.
timmylicheng commented on a change in pull request #1191:
URL: https://github.com/apache/hadoop-ozone/pull/1191#discussion_r464227674
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisServerImpl.java
##########
@@ -110,4 +111,18 @@ public void stop() throws IOException {
server.close();
}
+ @Override
+ public RaftServer getServer() {
+ return server;
+ }
+
+ @Override
+ public RaftGroupId getRaftGroupId() {
+ return raftGroupId;
+ }
+
+ @Override
+ public List<RaftPeer> getRaftPeers() {
+ return Collections.singletonList(new RaftPeer(raftPeerId));
Review comment:
Yes. And this class will be deprecated soon in SCM group feature.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] timmylicheng commented on a change in pull request #1191: HDDS-3837 Add isLeader check in SCMHAManager.
Posted by GitBox <gi...@apache.org>.
timmylicheng commented on a change in pull request #1191:
URL: https://github.com/apache/hadoop-ozone/pull/1191#discussion_r464420045
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAManagerImpl.java
##########
@@ -67,6 +101,44 @@ public SCMRatisServer getRatisServer() {
return ratisServer;
}
+ private RaftPeerId getPeerIdFromRoleInfo(RaftServerImpl serverImpl) {
+ if (serverImpl.isLeader()) {
+ return RaftPeerId.getRaftPeerId(
+ serverImpl.getRoleInfoProto().getLeaderInfo().toString());
+ } else if (serverImpl.isFollower()) {
+ return RaftPeerId.valueOf(
+ serverImpl.getRoleInfoProto().getFollowerInfo()
+ .getLeaderInfo().getId().getId());
+ } else {
+ return null;
+ }
+ }
+
+ @Override
+ public RaftPeer getSuggestedLeader() {
+ RaftServer server = ratisServer.getServer();
+ Preconditions.checkState(server instanceof RaftServerProxy);
+ RaftServerImpl serverImpl = null;
+ try {
+ // SCM only has one raft group.
+ serverImpl = ((RaftServerProxy) server)
+ .getImpl(ratisServer.getRaftGroupId());
+ } catch (IOException ioe) {
+ LOG.error("Fail to get RaftServer impl and therefore it's not clear " +
+ "whether it's leader. ", ioe);
+ } finally {
+ if (serverImpl != null) {
Review comment:
Updated
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] timmylicheng closed pull request #1191: HDDS-3837 Add isLeader check in SCMHAManager.
Posted by GitBox <gi...@apache.org>.
timmylicheng closed pull request #1191:
URL: https://github.com/apache/hadoop-ozone/pull/1191
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1191: HDDS-3837 Add isLeader check in SCMHAManager.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1191:
URL: https://github.com/apache/hadoop-ozone/pull/1191#discussion_r463349967
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NewNodeHandler.java
##########
@@ -41,6 +46,11 @@ public NewNodeHandler(PipelineManager pipelineManager,
@Override
public void onMessage(DatanodeDetails datanodeDetails,
EventPublisher publisher) {
- pipelineManager.triggerPipelineCreation();
+ try {
+ pipelineManager.triggerPipelineCreation();
+ } catch (NotLeaderException ex) {
+ LOG.warn("Not the current leader SCM and cannot start pipeline" +
Review comment:
Can we put this as debug log to avoid spamming the SCM log?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] timmylicheng closed pull request #1191: HDDS-3837 Add isLeader check in SCMHAManager.
Posted by GitBox <gi...@apache.org>.
timmylicheng closed pull request #1191:
URL: https://github.com/apache/hadoop-ozone/pull/1191
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on pull request #1191: HDDS-3837 Add isLeader check in SCMHAManager.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on pull request #1191:
URL: https://github.com/apache/hadoop-ozone/pull/1191#issuecomment-669624594
+1, Thanks @timmylicheng for the patience.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] timmylicheng closed pull request #1191: HDDS-3837 Add isLeader check in SCMHAManager.
Posted by GitBox <gi...@apache.org>.
timmylicheng closed pull request #1191:
URL: https://github.com/apache/hadoop-ozone/pull/1191
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] xiaoyuyao commented on a change in pull request #1191: HDDS-3837 Add isLeader check in SCMHAManager.
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1191:
URL: https://github.com/apache/hadoop-ozone/pull/1191#discussion_r461111835
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAManagerImpl.java
##########
@@ -67,6 +101,44 @@ public SCMRatisServer getRatisServer() {
return ratisServer;
}
+ private RaftPeerId getPeerIdFromRoleInfo(RaftServerImpl serverImpl) {
+ if (serverImpl.isLeader()) {
+ return RaftPeerId.getRaftPeerId(
+ serverImpl.getRoleInfoProto().getLeaderInfo().toString());
+ } else if (serverImpl.isFollower()) {
+ return RaftPeerId.getRaftPeerId(
+ serverImpl.getRoleInfoProto().getFollowerInfo()
+ .getLeaderInfo().getId().getAddress());
Review comment:
getId().getAddress() seems unnecessary.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] timmylicheng commented on a change in pull request #1191: HDDS-3837 Add isLeader check in SCMHAManager.
Posted by GitBox <gi...@apache.org>.
timmylicheng commented on a change in pull request #1191:
URL: https://github.com/apache/hadoop-ozone/pull/1191#discussion_r462010054
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAManagerImpl.java
##########
@@ -67,6 +101,44 @@ public SCMRatisServer getRatisServer() {
return ratisServer;
}
+ private RaftPeerId getPeerIdFromRoleInfo(RaftServerImpl serverImpl) {
+ if (serverImpl.isLeader()) {
+ return RaftPeerId.getRaftPeerId(
+ serverImpl.getRoleInfoProto().getLeaderInfo().toString());
+ } else if (serverImpl.isFollower()) {
+ return RaftPeerId.getRaftPeerId(
+ serverImpl.getRoleInfoProto().getFollowerInfo()
+ .getLeaderInfo().getId().getAddress());
Review comment:
I update with what OM does.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAManagerImpl.java
##########
@@ -56,7 +69,28 @@ public void start() throws IOException {
*/
@Override
public boolean isLeader() {
- return isLeader;
+ if (!SCMHAUtils.isSCMHAEnabled(conf)) {
+ // When SCM HA is not enabled, the current SCM is always the leader.
+ return true;
+ }
+ RaftServer server = ratisServer.getServer();
+ Preconditions.checkState(server instanceof RaftServerProxy);
+ RaftServerImpl serverImpl = null;
+ try {
+ // SCM only has one raft group.
+ serverImpl = ((RaftServerProxy) server)
+ .getImpl(ratisServer.getRaftGroupId());
+ } catch (IOException ioe) {
+ LOG.error("Fail to get RaftServer impl and therefore it's not clear " +
+ "whether it's leader. ", ioe);
+ } finally {
+ if (serverImpl != null) {
Review comment:
Done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org