You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by "AngersZhuuuu (via GitHub)" <gi...@apache.org> on 2023/03/01 07:28:24 UTC
[GitHub] [ratis] AngersZhuuuu opened a new pull request, #834: [RATIS-1798] ratis-shell command should respect GENERIC_COMMAND_OPTIONS
AngersZhuuuu opened a new pull request, #834:
URL: https://github.com/apache/ratis/pull/834
## What changes were proposed in this pull request?
Our ratis cluster use NETTY rpc, since default RPC is GRPC, when we want to use ratis shell to change leader
we use below command
```
ratis sh -Draft.rpc.type=nettu election transfer -peers ip-xxx-xxx-xxx-001.idata-server.shopee.io:9872,ip-xxx-xxx-xxx-002.idata-server.shopee.io:9872,ip-xxx-xxx-xxx-003.idata-server.shopee.io:9872 -groupid 3ab29dc7-61dd-3e52-9a8d-f9c189140641 -address ip-xxx-xxx-xxx-003.idata-server.shopee.io:9872
```
Found GENERIC_COMMAND_OPTIONS can't work well.
This pr resolve this issue.
## What is the link to the Apache JIRA
#RATIS-1798
Please replace this section with the link to the Apache JIRA)
## How was this patch tested?
Tested in our env
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] kaijchen commented on pull request #834: RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on PR #834:
URL: https://github.com/apache/ratis/pull/834#issuecomment-1451594356
> Thank @kaijchen for the information, yes, it works after this minor fix, I know you are active in the Ozone community, would you like to share more about why Ozone uses gRPC by default?
Basically gRPC is higher level and provides more features, while Netty is lower level and has less overhead.
There is a blog post about why Alluxio switch from Thrift + Netty to gRPC:
https://www.alluxio.io/blog/moving-from-apache-thrift-to-grpc-a-perspective-from-alluxio/
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] AngersZhuuuu commented on pull request #834: [RATIS-1798] ratis-shell command should respect GENERIC_COMMAND_OPTIONS
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on PR #834:
URL: https://github.com/apache/ratis/pull/834#issuecomment-1449482131
ping @SzyWilliam @kaijchen @tisonkun @codings-dan
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #834: RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #834:
URL: https://github.com/apache/ratis/pull/834#issuecomment-1452365539
> https://www.alluxio.io/blog/moving-from-apache-thrift-to-grpc-a-perspective-from-alluxio/
@kaijchen , thanks for sharing the blog!
> **Implement zero-copy**: Another overhead we observed switching to gRPC is excessive buffer copying. This is because gRPC, by default, uses protobuf for message serialization, which introduces extra copy operations. There is currently no official API for getting raw buffers in and out of gRPC without having to go through protobuf serialization or throw away the nice generated RPC bindings. We end up using some ideas from the gRPC user group and implemented a workaround to achieve zero-copy.
Do you have more details on zero-copy? I wonder if we could do the same in Ratis.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] kaijchen commented on pull request #834: RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on PR #834:
URL: https://github.com/apache/ratis/pull/834#issuecomment-1452849886
> Do you have more details on zero-copy? I wonder if we could do the same in Ratis.
https://github.com/Alluxio/alluxio/pull/8353
https://github.com/Alluxio/alluxio/pull/8526
You may also check:
https://github.com/GoogleCloudPlatform/grpc-gcp-java/pull/77
https://github.com/grpc/grpc-java/pull/8102
AFAIK, it is still not as good as Netty though.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] pan3793 commented on pull request #834: RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #834:
URL: https://github.com/apache/ratis/pull/834#issuecomment-1452829079
@szetszwo, thank you for sharing the detail, based on your description, the gRPC should be the best choice for Apache Celeborn(Incubating).
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a diff in pull request #834: RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #834:
URL: https://github.com/apache/ratis/pull/834#discussion_r1122215687
##########
ratis-shell/src/main/java/org/apache/ratis/shell/cli/RaftUtils.java:
##########
@@ -65,6 +65,10 @@ public static RaftPeerId getPeerId(String host, int port) {
*/
public static RaftClient createClient(RaftGroup raftGroup) {
RaftProperties properties = new RaftProperties();
+ // Since ratis-shell support GENERIC_COMMAND_OPTIONS, here we should
+ // set these options to raft properties to make it work.
+ System.getProperties().stringPropertyNames().forEach(
+ key -> properties.setIfUnset(key, System.getProperty(key)));
RaftClientConfigKeys.Rpc.setRequestTimeout(properties,
Review Comment:
@AngersZhuuuu , thanks for working on this! This is a good change. We should check if the requestTimeout is already set.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] pan3793 commented on pull request #834: RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #834:
URL: https://github.com/apache/ratis/pull/834#issuecomment-1451488577
May I ask which RPC implementation is favored in the downstream projects or production cases? Are there general pros and cons of each one?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] AngersZhuuuu commented on pull request #834: RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on PR #834:
URL: https://github.com/apache/ratis/pull/834#issuecomment-1450221462
The failed test seems not related
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] SzyWilliam commented on pull request #834: RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS
Posted by "SzyWilliam (via GitHub)" <gi...@apache.org>.
SzyWilliam commented on PR #834:
URL: https://github.com/apache/ratis/pull/834#issuecomment-1451728227
> https://www.alluxio.io/blog/moving-from-apache-thrift-to-grpc-a-perspective-from-alluxio/
@kaijchen The link seems broken. IoTDB community also had a discussion about Thrift v.s gRPC recently. It would be great to learn lessons from other projects.
@pan3793 IoTDB also uses gRPC by default. It was originally a "captain's call" ;). gRPC mostly meets our needs. However, we are experiencing [issues](https://issues.apache.org/jira/browse/RATIS-1790) related to gRPC's installSnapshot process recently.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #834: RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #834:
URL: https://github.com/apache/ratis/pull/834#issuecomment-1452879718
@kaijchen , thanks for the info. Will check them.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] pan3793 commented on pull request #834: RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #834:
URL: https://github.com/apache/ratis/pull/834#issuecomment-1451597524
Many thanks, @kaijchen
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] kaijchen commented on pull request #834: RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on PR #834:
URL: https://github.com/apache/ratis/pull/834#issuecomment-1451792893
> The link seems broken. IoTDB community also had a discussion about Thrift v.s gRPC recently. It would be great to learn lessons from other projects.
You need to set language to EN first: https://www.alluxio.io/?lang=en
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a diff in pull request #834: RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #834:
URL: https://github.com/apache/ratis/pull/834#discussion_r1123659267
##########
ratis-shell/src/main/java/org/apache/ratis/shell/cli/RaftUtils.java:
##########
@@ -64,9 +64,13 @@ public static RaftPeerId getPeerId(String host, int port) {
* @return return a raft client
*/
public static RaftClient createClient(RaftGroup raftGroup) {
- RaftProperties properties = new RaftProperties();
- RaftClientConfigKeys.Rpc.setRequestTimeout(properties,
+ RaftProperties defaults = new RaftProperties();
+ RaftClientConfigKeys.Rpc.setRequestTimeout(defaults,
TimeDuration.valueOf(15, TimeUnit.SECONDS));
+
+ // Since ratis-shell support GENERIC_COMMAND_OPTIONS, here we should
+ // merge these options to raft properties to make it work.
+ RaftProperties systems = new RaftProperties(System.getProperties());
Review Comment:
@AngersZhuuuu , let's simply use `forEach` instead of changing `RaftProperties`.
```java
@@ -67,6 +68,12 @@ public final class RaftUtils {
RaftProperties properties = new RaftProperties();
RaftClientConfigKeys.Rpc.setRequestTimeout(properties,
TimeDuration.valueOf(15, TimeUnit.SECONDS));
+
+ // Since ratis-shell support GENERIC_COMMAND_OPTIONS, here we should
+ // merge these options to raft properties to make it work.
+ final Properties sys = System.getProperties();
+ sys.stringPropertyNames().forEach(key -> properties.set(key, sys.getProperty(key)));
+
ExponentialBackoffRetry retryPolicy = ExponentialBackoffRetry.newBuilder()
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] kaijchen commented on pull request #834: RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on PR #834:
URL: https://github.com/apache/ratis/pull/834#issuecomment-1451557584
> Actually, this issue is found in the Celeborn project when we want to include `ratis-shell` in. A direct manifestation of this issue is Celeborn uses the Netty RPC implementation but ratis-shell does not support that.
`ratis-shell` is originally contributed by Ozone and Alluxio community, these two projects use gRPC by default.
https://issues.apache.org/jira/browse/RATIS-1412
But Netty should be supported as well. Glad to see we have Netty use cases in Celeborn.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] pan3793 commented on pull request #834: RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #834:
URL: https://github.com/apache/ratis/pull/834#issuecomment-1451569371
Thank @kaijchen for the information, yes, it works after this minor fix, I know you are active in the Ozone community, would you like to share more about why Ozone uses gRPC by default?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #834: RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #834:
URL: https://github.com/apache/ratis/pull/834#issuecomment-1452337900
> May I ask which RPC implementation is favored in the downstream projects or production cases? Are there general pros and cons of each one?
@pan3793 , thanks for being interested in Apache Ratis! I agree with @tisonkun that this kind of questions is better to discuss it on the mailing list. Let me give you some short answer.
Currently, grpc is the default rpc type in Ratis. It supports all the functionalities -- blocking io, ordered async and unordered async. Netty RPC in Ratis only supports blocking io.
From our experience, grpc is good when the message sizes are small. For large messages, grpc does not perform well since we cannot control the buffer allocation and copying. Netty is better in that case.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] pan3793 commented on pull request #834: RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #834:
URL: https://github.com/apache/ratis/pull/834#issuecomment-1451499170
> This question is unrelated to this patch.
Actually, this issue is found in the Celeborn project when we want to include `ratis-shell` in. A direct manifestation of the problem is Celeborn uses the Netty RPC implementation but ratis-shell does not support that.
@tisonkun Based on this fact, I think they are kind of "related".
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] pan3793 commented on pull request #834: RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #834:
URL: https://github.com/apache/ratis/pull/834#issuecomment-1451733627
@SzyWilliam thanks for the information, looks like the gRPC protocol has more practices.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] AngersZhuuuu commented on a diff in pull request #834: RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #834:
URL: https://github.com/apache/ratis/pull/834#discussion_r1123963697
##########
ratis-shell/src/main/java/org/apache/ratis/shell/cli/RaftUtils.java:
##########
@@ -64,9 +64,13 @@ public static RaftPeerId getPeerId(String host, int port) {
* @return return a raft client
*/
public static RaftClient createClient(RaftGroup raftGroup) {
- RaftProperties properties = new RaftProperties();
- RaftClientConfigKeys.Rpc.setRequestTimeout(properties,
+ RaftProperties defaults = new RaftProperties();
+ RaftClientConfigKeys.Rpc.setRequestTimeout(defaults,
TimeDuration.valueOf(15, TimeUnit.SECONDS));
+
+ // Since ratis-shell support GENERIC_COMMAND_OPTIONS, here we should
+ // merge these options to raft properties to make it work.
+ RaftProperties systems = new RaftProperties(System.getProperties());
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] AngersZhuuuu commented on a diff in pull request #834: RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #834:
URL: https://github.com/apache/ratis/pull/834#discussion_r1122535641
##########
ratis-shell/src/main/java/org/apache/ratis/shell/cli/RaftUtils.java:
##########
@@ -65,6 +65,10 @@ public static RaftPeerId getPeerId(String host, int port) {
*/
public static RaftClient createClient(RaftGroup raftGroup) {
RaftProperties properties = new RaftProperties();
+ // Since ratis-shell support GENERIC_COMMAND_OPTIONS, here we should
+ // set these options to raft properties to make it work.
+ System.getProperties().stringPropertyNames().forEach(
+ key -> properties.setIfUnset(key, System.getProperty(key)));
RaftClientConfigKeys.Rpc.setRequestTimeout(properties,
Review Comment:
> We should check if the requestTimeout is already set.
How about the current change? Properties in `System` should have higher priority. It can cover the requestTimeout if we set it passed by `-D`
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] tisonkun commented on pull request #834: RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #834:
URL: https://github.com/apache/ratis/pull/834#issuecomment-1451491138
@pan3793 This question is unrelated to this patch. I suggest you email to [user@ratis.apache.org](mailto:user@ratis.apache.org) for discussion. See [this page](https://ratis.apache.org/#community) about how to subscribe the list.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo merged pull request #834: RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #834:
URL: https://github.com/apache/ratis/pull/834
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org