RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS #834

AngersZhuuuu · 2023-03-01T07:28:22Z

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=netty 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

https://issues.apache.org/jira/browse/RATIS-1798

Please replace this section with the link to the Apache JIRA)

How was this patch tested?

Tested in our env

AngersZhuuuu · 2023-03-01T07:29:33Z

ping @SzyWilliam @kaijchen @tisonkun @codings-dan

kaijchen

LGTM, thanks @AngersZhuuuu.

codings-dan

Thanks for working on this! the change looks good, +1

AngersZhuuuu · 2023-03-01T14:14:19Z

The failed test seems not related

szetszwo · 2023-03-01T19:35:44Z

ratis-shell/src/main/java/org/apache/ratis/shell/cli/RaftUtils.java

+    // 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,


@AngersZhuuuu , thanks for working on this! This is a good change. We should check if the requestTimeout is already set.

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

szetszwo

+1 the change looks good.

tisonkun

Generally looks good. One comment here:

It can be better to support properties merge for RaftProperties, like:

diff --git a/ratis-common/src/main/java/org/apache/ratis/conf/RaftProperties.java b/ratis-common/src/main/java/org/apache/ratis/conf/RaftProperties.java
index f51bc731..a0f19899 100644
--- a/ratis-common/src/main/java/org/apache/ratis/conf/RaftProperties.java
+++ b/ratis-common/src/main/java/org/apache/ratis/conf/RaftProperties.java
@@ -18,6 +18,8 @@
 
 package org.apache.ratis.conf;
 
+import java.util.Properties;
+import org.apache.ratis.thirdparty.com.google.common.collect.Maps;
 import org.apache.ratis.util.JavaUtils;
 import org.apache.ratis.util.ReflectionUtils;
 import org.apache.ratis.util.SizeInBytes;
@@ -53,6 +55,10 @@ public class RaftProperties {
   public RaftProperties() {
   }
 
+  public RaftProperties(Properties properties) {
+    this.properties.putAll(Maps.fromProperties(properties));
+  }
+
   /**
    * A new RaftProperties with the same settings cloned from another.
    *
@@ -124,6 +130,24 @@ public class RaftProperties {
     return result;
   }
 
+  /**
+   * Merge two raft properties.
+   *
+   * <p>
+   * Properties in {@code p2} overwrite those in {@code p1} if they have
+   * the same key.
+   * </p>
+   *
+   * @param p1 first properties
+   * @param p2 second properties
+   * @return merged properties
+   */
+  public static RaftProperties merge(RaftProperties p1, RaftProperties p2) {
+    RaftProperties result = new RaftProperties(p1);
+    result.properties.putAll(p2.properties);
+    return result;
+  }
+
   /**
    * Attempts to repeatedly expand the value {@code expr} by replacing the
    * left-most substring of the form "${var}" in the following precedence order
diff --git a/ratis-shell/src/main/java/org/apache/ratis/shell/cli/RaftUtils.java b/ratis-shell/src/main/java/org/apache/ratis/shell/cli/RaftUtils.java
index dfbfdd32..6881b4a6 100644
--- a/ratis-shell/src/main/java/org/apache/ratis/shell/cli/RaftUtils.java
+++ b/ratis-shell/src/main/java/org/apache/ratis/shell/cli/RaftUtils.java
@@ -64,8 +64,8 @@ public final class RaftUtils {
    * @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));
     ExponentialBackoffRetry retryPolicy = ExponentialBackoffRetry.newBuilder()
         .setBaseSleepTime(TimeDuration.valueOf(1000, TimeUnit.MILLISECONDS))
@@ -73,9 +73,12 @@ public final class RaftUtils {
         .setMaxSleepTime(
             TimeDuration.valueOf(100_000, TimeUnit.MILLISECONDS))
         .build();
+
+    RaftProperties systems = new RaftProperties(System.getProperties());
+
     return RaftClient.newBuilder()
         .setRaftGroup(raftGroup)
-        .setProperties(properties)
+        .setProperties(RaftProperties.merge(defaults, systems))
         .setRetryPolicy(retryPolicy)
         .build();
   }

But it's more on style preference, so committers make the call.

This reverts commit 70c0a8f.

pan3793 · 2023-03-02T08:35:10Z

May I ask which RPC implementation is favored in the downstream projects or production cases? Are there general pros and cons of each one?

tisonkun · 2023-03-02T08:37:20Z

@pan3793 This question is unrelated to this patch. I suggest you email to [email protected] for discussion. See this page about how to subscribe the list.

pan3793 · 2023-03-02T08:44:06Z

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 this issue 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".

kaijchen · 2023-03-02T09:26:25Z

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.

pan3793 · 2023-03-02T09:35:02Z

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?

kaijchen · 2023-03-02T09:54:28Z

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/

pan3793 · 2023-03-02T09:56:53Z

Many thanks, @kaijchen

SzyWilliam · 2023-03-02T11:37:32Z

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 related to gRPC's installSnapshot process recently.

pan3793 · 2023-03-02T11:42:26Z

@SzyWilliam thanks for the information, looks like the gRPC protocol has more practices.

BTW, I can open the link

https://www.alluxio.io/blog/moving-from-apache-thrift-to-grpc-a-perspective-from-alluxio/

kaijchen · 2023-03-02T12:30:52Z

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

szetszwo · 2023-03-02T18:24:00Z

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.

szetszwo · 2023-03-02T18:40:58Z

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.

szetszwo · 2023-03-02T20:29:15Z

ratis-shell/src/main/java/org/apache/ratis/shell/cli/RaftUtils.java

+
+    // 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());


@AngersZhuuuu , let's simply use forEach instead of changing RaftProperties.

@@ -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()

pan3793 · 2023-03-03T02:01:23Z

@szetszwo, thank you for sharing the detail, based on your description, the gRPC should be the best choice for Apache Celeborn(Incubating).

kaijchen · 2023-03-03T02:15:25Z

Do you have more details on zero-copy? I wonder if we could do the same in Ratis.

Alluxio/alluxio#8353
Alluxio/alluxio#8526

You may also check:

GoogleCloudPlatform/grpc-gcp-java#77
grpc/grpc-java#8102

AFAIK, it is still not as good as Netty though.

szetszwo · 2023-03-03T02:50:10Z

@kaijchen , thanks for the info. Will check them.

szetszwo

+1 the change looks good.

…834) (cherry picked from commit b5f9c0f)

…pache#834)

[RATIS-1798] ratis-shell command should respect GENERIC_COMMAND_OPTIONS

2ac93d5

kaijchen approved these changes Mar 1, 2023

View reviewed changes

codings-dan approved these changes Mar 1, 2023

View reviewed changes

codings-dan changed the title ~~[RATIS-1798] ratis-shell command should respect GENERIC_COMMAND_OPTIONS~~ RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS Mar 1, 2023

szetszwo reviewed Mar 1, 2023

View reviewed changes

Update RaftUtils.java

41867bd

szetszwo approved these changes Mar 2, 2023

View reviewed changes

tisonkun reviewed Mar 2, 2023

View reviewed changes

AngersZhuuuu added 4 commits March 2, 2023 14:45

follow comment

a237252

Merge branch 'master' into RATIS-1798

45292d6

Update TestLogAppenderWithGrpc.java

70c0a8f

Revert "Update TestLogAppenderWithGrpc.java"

7d61e2c

This reverts commit 70c0a8f.

tisonkun approved these changes Mar 2, 2023

View reviewed changes

szetszwo requested changes Mar 2, 2023

View reviewed changes

update

c6d83d0

szetszwo approved these changes Mar 3, 2023

View reviewed changes

szetszwo merged commit b5f9c0f into apache:master Mar 3, 2023

szetszwo pushed a commit that referenced this pull request Mar 5, 2023

RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS (#…

12e0c6e

…834) (cherry picked from commit b5f9c0f)

symious pushed a commit to symious/ratis that referenced this pull request Mar 16, 2024

RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS (a…

71252ed

…pache#834)

RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS #834

RATIS-1798. Make ratis-shell command respect GENERIC_COMMAND_OPTIONS #834

Uh oh!

Conversation

AngersZhuuuu commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

AngersZhuuuu commented Mar 1, 2023

Uh oh!

kaijchen left a comment

Choose a reason for hiding this comment

Uh oh!

codings-dan left a comment

Choose a reason for hiding this comment

Uh oh!

AngersZhuuuu commented Mar 1, 2023

Uh oh!

szetszwo Mar 1, 2023

Choose a reason for hiding this comment

Uh oh!

AngersZhuuuu Mar 2, 2023

Choose a reason for hiding this comment

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

tisonkun left a comment

Choose a reason for hiding this comment

Uh oh!

pan3793 commented Mar 2, 2023

Uh oh!

tisonkun commented Mar 2, 2023

Uh oh!

pan3793 commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaijchen commented Mar 2, 2023

Uh oh!

pan3793 commented Mar 2, 2023

Uh oh!

kaijchen commented Mar 2, 2023

Uh oh!

pan3793 commented Mar 2, 2023

Uh oh!

SzyWilliam commented Mar 2, 2023

Uh oh!

pan3793 commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaijchen commented Mar 2, 2023

Uh oh!

szetszwo commented Mar 2, 2023

Uh oh!

szetszwo commented Mar 2, 2023

Uh oh!

szetszwo Mar 2, 2023

Choose a reason for hiding this comment

Uh oh!

AngersZhuuuu Mar 3, 2023

Choose a reason for hiding this comment

Uh oh!

pan3793 commented Mar 3, 2023

Uh oh!

kaijchen commented Mar 3, 2023

Uh oh!

szetszwo commented Mar 3, 2023

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

AngersZhuuuu commented Mar 1, 2023 •

edited

Loading

pan3793 commented Mar 2, 2023 •

edited

Loading

pan3793 commented Mar 2, 2023 •

edited

Loading