You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ratis.apache.org by GitBox <gi...@apache.org> on 2020/11/25 23:26:15 UTC
[GitHub] [incubator-ratis] bharatviswa504 opened a new pull request #300: RATIS-1180. Expose RaftServer status with Status Info
bharatviswa504 opened a new pull request #300:
URL: https://github.com/apache/incubator-ratis/pull/300
## What changes were proposed in this pull request?
Right now we have isLeader() but it says false when it is leader and not ready. There is no way to figure out if it is leader and not ready from this API, we need to call isLeader and isLeaderReady for that.
This Jira exposes the ServerStatus with the following info.
NOT_LEADER, LEADER_AND_READY, and LEADER_AND_NOT_READY.
The purpose of Jira is to avoid multiple calls like isLeader and isLeaderReady, trying to achieve the same with a single API.
Expose ServerStatus API with the following info.
NOT_LEADER, LEADER_AND_READY, and LEADER_AND_NOT_READY.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-1180
## How was this patch tested?
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] bharatviswa504 commented on a change in pull request #300: RATIS-1180. Expose RaftServer status with Status Info
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #300:
URL: https://github.com/apache/incubator-ratis/pull/300#discussion_r530719682
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -593,6 +593,30 @@ public boolean isLeaderReady() {
return true;
}
+ /**
+ * Return status of the RaftServer.
+ * @return LeaderStatus
+ */
+ public ServerStatus getServerStatus() {
Review comment:
Because in OM HA, we want to check isLeaderReady and then start serving read requests.
https://issues.apache.org/jira/browse/HDDS-4484
The main reason was not to call multiple time getImpl(groupID) and then find isLeader and isLeaderReady.
We use RaftServer Builder build() to construct and use that object to make calls to Ratis.
```
this.server = RaftServer.newBuilder()
.setServerId(this.raftPeerId)
.setGroup(this.raftGroup)
.setProperties(serverProperties)
.setStateMachine(omStateMachine)
.build();
```
Let me know what would be the best way to expose this information from Ratis? By adding it to RaftServer?
https://issues.apache.org/jira/browse/HDDS-4484 Jira in Ozone for OM HA.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] bharatviswa504 commented on a change in pull request #300: RATIS-1180. Expose RaftServer status with Status Info
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #300:
URL: https://github.com/apache/incubator-ratis/pull/300#discussion_r530714897
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -593,6 +593,30 @@ public boolean isLeaderReady() {
return true;
}
+ /**
+ * Return status of the RaftServer.
+ * @return LeaderStatus
+ */
+ public ServerStatus getServerStatus() {
Review comment:
There is a slight difference, isLeaderReady will return false when it is leader but not ready, this method helps in finding that.
isLeaderReady -> True when leader and ready
-> false when not leader
-> false when leader and not ready
This API can help can let the caller know exactly the server status.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] bharatviswa504 closed pull request #300: RATIS-1180. Expose RaftServer status with Status Info
Posted by GitBox <gi...@apache.org>.
bharatviswa504 closed pull request #300:
URL: https://github.com/apache/incubator-ratis/pull/300
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] bharatviswa504 commented on pull request #300: RATIS-1180. Expose RaftServer status with Status Info
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #300:
URL: https://github.com/apache/incubator-ratis/pull/300#issuecomment-742771551
Closing this, it is taken care under subtasks of RATIS-1181.
Thank You @szetszwo for providing new APIs.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] szetszwo commented on a change in pull request #300: RATIS-1180. Expose RaftServer status with Status Info
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #300:
URL: https://github.com/apache/incubator-ratis/pull/300#discussion_r530727080
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -593,6 +593,30 @@ public boolean isLeaderReady() {
return true;
}
+ /**
+ * Return status of the RaftServer.
+ * @return LeaderStatus
+ */
+ public ServerStatus getServerStatus() {
Review comment:
Could you use the following code in Ozone for the moment?
```
final RaftServerImpl impl = ((RaftServerProxy)raftServer).getImpl(groupId);
if (!impl.isLeader()) {
// not leader
} else if (impl.isLeaderReady()) {
// leader and ready
} else {
// leader but not ready
}
```
I will work on creating new APIs.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] szetszwo commented on a change in pull request #300: RATIS-1180. Expose RaftServer status with Status Info
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #300:
URL: https://github.com/apache/incubator-ratis/pull/300#discussion_r530713609
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -593,6 +593,30 @@ public boolean isLeaderReady() {
return true;
}
+ /**
+ * Return status of the RaftServer.
+ * @return LeaderStatus
+ */
+ public ServerStatus getServerStatus() {
Review comment:
Do we really need this? This method is almost the same as isLeaderReady().
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] bharatviswa504 commented on pull request #300: RATIS-1180. Expose RaftServer status with Status Info
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #300:
URL: https://github.com/apache/incubator-ratis/pull/300#issuecomment-734431840
> @bharatviswa504 , just have filed https://issues.apache.org/jira/browse/HDDS-4517 and https://issues.apache.org/jira/browse/RATIS-1181 .
Thank You @szetszwo . I will try to use the new APIs introduced once RATIS-1181 is over.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] szetszwo commented on a change in pull request #300: RATIS-1180. Expose RaftServer status with Status Info
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #300:
URL: https://github.com/apache/incubator-ratis/pull/300#discussion_r530717797
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -593,6 +593,30 @@ public boolean isLeaderReady() {
return true;
}
+ /**
+ * Return status of the RaftServer.
+ * @return LeaderStatus
+ */
+ public ServerStatus getServerStatus() {
Review comment:
We already have isLeader() and isLeaderReady() covering all the cases. Could you use these two methods for now?
RaftServerImpl is an internal Ratis class but not a public API. Ozone should not use it directly. If there is a need, we should define a public API but not adding methods to RaftServerImpl.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] bharatviswa504 commented on a change in pull request #300: RATIS-1180. Expose RaftServer status with Status Info
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #300:
URL: https://github.com/apache/incubator-ratis/pull/300#discussion_r530715499
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -593,6 +593,30 @@ public boolean isLeaderReady() {
return true;
}
+ /**
+ * Return status of the RaftServer.
+ * @return LeaderStatus
+ */
+ public ServerStatus getServerStatus() {
Review comment:
One reason is in OM HA we want to check isLeader and isLeaderReady and these are implemented as below.
public boolean isLeader() {
Preconditions.checkState(server instanceof RaftServerProxy);
RaftServerImpl serverImpl = null;
try {
serverImpl = ((RaftServerProxy) server).getImpl(raftGroupId);
if (serverImpl != null) {
return serverImpl.isLeader();
}
} catch (IOException ioe) {
// In this case we return not a leader.
LOG.error("Fail to get RaftServer impl and therefore it's not clear " +
"whether it's leader. ", ioe);
}
return false;
}
/**
* Return true, if the current OM node is leader and in ready state to
* process the requests.
* @return
*/
public boolean isLeaderReady() {
Preconditions.checkState(server instanceof RaftServerProxy);
RaftServerImpl serverImpl = null;
try {
serverImpl = ((RaftServerProxy) server).getImpl(raftGroupId);
if (serverImpl != null) {
return serverImpl.isLeaderReady();
}
} catch (IOException ioe) {
// In this case we return not a leader.
LOG.error("Fail to get RaftServer impl and therefore it's not clear " +
"whether it's leader. ", ioe);
}
return false;
}
With this new API, we can call the new API and know when it is NOT_LEADER, LEADER_AND_READY and LEADER_AND_NOT_READY
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] bharatviswa504 edited a comment on pull request #300: RATIS-1180. Expose RaftServer status with Status Info
Posted by GitBox <gi...@apache.org>.
bharatviswa504 edited a comment on pull request #300:
URL: https://github.com/apache/incubator-ratis/pull/300#issuecomment-734431840
> @bharatviswa504 , just have filed https://issues.apache.org/jira/browse/HDDS-4517 and https://issues.apache.org/jira/browse/RATIS-1181 .
Thank You @szetszwo . I will try to use the new APIs which will be introduced as part of RATIS-1181.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] szetszwo commented on a change in pull request #300: RATIS-1180. Expose RaftServer status with Status Info
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #300:
URL: https://github.com/apache/incubator-ratis/pull/300#discussion_r530717956
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -593,6 +593,30 @@ public boolean isLeaderReady() {
return true;
}
+ /**
+ * Return status of the RaftServer.
+ * @return LeaderStatus
+ */
+ public ServerStatus getServerStatus() {
Review comment:
BTW, RaftServerProxy is also an internal Ratis class.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] bharatviswa504 commented on a change in pull request #300: RATIS-1180. Expose RaftServer status with Status Info
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #300:
URL: https://github.com/apache/incubator-ratis/pull/300#discussion_r530719682
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -593,6 +593,30 @@ public boolean isLeaderReady() {
return true;
}
+ /**
+ * Return status of the RaftServer.
+ * @return LeaderStatus
+ */
+ public ServerStatus getServerStatus() {
Review comment:
Because in OM HA, we want to check isLeaderReady and then start serving read requests.
https://issues.apache.org/jira/browse/HDDS-4484
The main reason was not to call multiple time getImpl(groupID), as now isLeader and isLeaderReady both need to call getImpl. When this proposed API, we can only once getImpl(groupID) and get server status.
We use RaftServer Builder build() to construct and use that object to make calls to Ratis.
```
this.server = RaftServer.newBuilder()
.setServerId(this.raftPeerId)
.setGroup(this.raftGroup)
.setProperties(serverProperties)
.setStateMachine(omStateMachine)
.build();
```
Let me know what would be the proper way to expose this information from Ratis? By adding it to RaftServer?
https://issues.apache.org/jira/browse/HDDS-4484 Jira in Ozone for OM HA.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] szetszwo commented on pull request #300: RATIS-1180. Expose RaftServer status with Status Info
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #300:
URL: https://github.com/apache/incubator-ratis/pull/300#issuecomment-734034426
@bharatviswa504 , just have filed https://issues.apache.org/jira/browse/HDDS-4517 and https://issues.apache.org/jira/browse/RATIS-1181 .
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] bharatviswa504 commented on a change in pull request #300: RATIS-1180. Expose RaftServer status with Status Info
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #300:
URL: https://github.com/apache/incubator-ratis/pull/300#discussion_r530719682
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -593,6 +593,30 @@ public boolean isLeaderReady() {
return true;
}
+ /**
+ * Return status of the RaftServer.
+ * @return LeaderStatus
+ */
+ public ServerStatus getServerStatus() {
Review comment:
Because in OM HA, we want to check isLeaderReady and then start serving read requests.
https://issues.apache.org/jira/browse/HDDS-4484
The main reason was not to call multiple time getImpl(groupID), as now isLeader and isLeaderReady both need to call getImpl. When this proposed API, we can only once getImpl(groupID) and get server status.
We use RaftServer Builder build() to construct and use that object to make calls to Ratis.
```
this.server = RaftServer.newBuilder()
.setServerId(this.raftPeerId)
.setGroup(this.raftGroup)
.setProperties(serverProperties)
.setStateMachine(omStateMachine)
.build();
```
Let me know what would be the best way to expose this information from Ratis? By adding it to RaftServer?
https://issues.apache.org/jira/browse/HDDS-4484 Jira in Ozone for OM HA.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] bharatviswa504 commented on a change in pull request #300: RATIS-1180. Expose RaftServer status with Status Info
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #300:
URL: https://github.com/apache/incubator-ratis/pull/300#discussion_r530715499
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -593,6 +593,30 @@ public boolean isLeaderReady() {
return true;
}
+ /**
+ * Return status of the RaftServer.
+ * @return LeaderStatus
+ */
+ public ServerStatus getServerStatus() {
Review comment:
One reason is in OM HA we want to check isLeader and isLeaderReady and these are implemented as below.
public boolean isLeader() {
Preconditions.checkState(server instanceof RaftServerProxy);
RaftServerImpl serverImpl = null;
try {
serverImpl = ((RaftServerProxy) server).getImpl(raftGroupId);
if (serverImpl != null) {
return serverImpl.isLeader();
}
} catch (IOException ioe) {
// In this case we return not a leader.
LOG.error("Fail to get RaftServer impl and therefore it's not clear " +
"whether it's leader. ", ioe);
}
return false;
}
/**
* Return true, if the current OM node is leader and in ready state to
* process the requests.
* @return
*/
public boolean isLeaderReady() {
Preconditions.checkState(server instanceof RaftServerProxy);
RaftServerImpl serverImpl = null;
try {
serverImpl = ((RaftServerProxy) server).getImpl(raftGroupId);
if (serverImpl != null) {
return serverImpl.isLeaderReady();
}
} catch (IOException ioe) {
// In this case we return not a leader.
LOG.error("Fail to get RaftServer impl and therefore it's not clear " +
"whether it's leader. ", ioe);
}
return false;
}
With this API i can call new API and return error codes to the client accordingly for retry.
----------------------------------------------------------------
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