You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "onebox-li (via GitHub)" <gi...@apache.org> on 2023/05/31 13:04:47 UTC

[GitHub] [incubator-celeborn] onebox-li opened a new pull request, #1530: [CELEBORN-623] Ratis-shell add default rpc type

onebox-li opened a new pull request, #1530:
URL: https://github.com/apache/incubator-celeborn/pull/1530

   
   ### What changes were proposed in this pull request?
   Ratis-shell use GRPC by default. Celeborn support Netty for ratis, if `raft.rpc.type` is not specified, commands may fail.
   e.g.
   ```
   org.apache.ratis.thirdparty.io.grpc.StatusRuntimeException: DEADLINE_EXCEEDED: deadline exceeded after 14.947369960s. [closed=[], open=[[buffered_nanos=14962358255, waiting_for_connection]]] 
   ```
   So i think we can add default netty rpc type in `celeborn-ratis` for better use.
   
   
   ### Why are the changes needed?
   Improve experience
   
   
   ### Does this PR introduce _any_ user-facing change?
   No
   
   
   ### How was this patch tested?
   Manually test
   


-- 
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@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] codecov[bot] commented on pull request #1530: [CELEBORN-623] Ratis-shell add default rpc type

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #1530:
URL: https://github.com/apache/incubator-celeborn/pull/1530#issuecomment-1570259849

   ## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1530?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   > Merging [#1530](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1530?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (391577a) into [main](https://app.codecov.io/gh/apache/incubator-celeborn/commit/fa920ab0d54d4c415bc00eb794c43f5f946f7151?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (fa920ab) will **decrease** coverage by `0.15%`.
   > The diff coverage is `n/a`.
   
   ```diff
   @@            Coverage Diff             @@
   ##             main    #1530      +/-   ##
   ==========================================
   - Coverage   46.07%   45.92%   -0.15%     
   ==========================================
     Files         158      158              
     Lines        9880     9880              
     Branches      967      967              
   ==========================================
   - Hits         4551     4536      -15     
   - Misses       5031     5043      +12     
   - Partials      298      301       +3     
   ```
   
   
   [see 1 file with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1530/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   


-- 
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@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] onebox-li commented on pull request #1530: [CELEBORN-623][DOCS] Document how to change RPC type in `ratis-shell`

Posted by "onebox-li (via GitHub)" <gi...@apache.org>.
onebox-li commented on PR #1530:
URL: https://github.com/apache/incubator-celeborn/pull/1530#issuecomment-1573649210

   Thanks for the guidance


-- 
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@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] onebox-li commented on pull request #1530: [CELEBORN-623] Ratis-shell add default rpc type

Posted by "onebox-li (via GitHub)" <gi...@apache.org>.
onebox-li commented on PR #1530:
URL: https://github.com/apache/incubator-celeborn/pull/1530#issuecomment-1571311823

   i will put this config into ratis shell's doc like @AngersZhuuuu said.


-- 
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@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1530: [CELEBORN-623] Ratis-shell add default rpc type

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1530:
URL: https://github.com/apache/incubator-celeborn/pull/1530#discussion_r1214300897


##########
docs/celeborn_ratis_shell.md:
##########
@@ -70,6 +70,7 @@ It support the following content:
 ```
 $ celeborn-ratis sh -D<property=value> ...
 ```
+For example: Use `-Draft.rpc.type=NETTY` to set the rpc type of ratis to netty, the default is grpc.

Review Comment:
   ```suggestion
   
   For example: use `-Draft.rpc.type=NETTY` to set the RPC type of Ratis to Netty, the default is gRPC.
   ```



-- 
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@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] onebox-li commented on a diff in pull request #1530: [CELEBORN-623] Ratis-shell add default rpc type

Posted by "onebox-li (via GitHub)" <gi...@apache.org>.
onebox-li commented on code in PR #1530:
URL: https://github.com/apache/incubator-celeborn/pull/1530#discussion_r1212546348


##########
bin/celeborn-ratis:
##########
@@ -71,6 +71,8 @@ function main {
   CELEBORN_RATIS_SHELL_JAVA_OPTS+=" -Dorg.apache.jasper.compiler.disablejsr199=true"
   CELEBORN_RATIS_SHELL_JAVA_OPTS+=" -Djava.net.preferIPv4Stack=true"
   CELEBORN_RATIS_SHELL_JAVA_OPTS+=" -Dorg.apache.ratis.thirdparty.io.netty.allocator.useCacheForAllThreads=false"
+  # Netty is supported by default
+  CELEBORN_RATIS_SHELL_JAVA_OPTS+=" -Draft.rpc.type=NETTY"

Review Comment:
   ok, get it.



-- 
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@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] waitinfuture commented on pull request #1530: [CELEBORN-623] Ratis-shell add default rpc type

Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #1530:
URL: https://github.com/apache/incubator-celeborn/pull/1530#issuecomment-1570243234

   LGTM, cc @FMX @AngersZhuuuu 


-- 
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@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1530: [CELEBORN-623] Ratis-shell add default rpc type

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1530:
URL: https://github.com/apache/incubator-celeborn/pull/1530#discussion_r1214300438


##########
docs/celeborn_ratis_shell.md:
##########
@@ -70,6 +70,7 @@ It support the following content:
 ```
 $ celeborn-ratis sh -D<property=value> ...
 ```
+For example: Using `-Draft.rpc.type=NETTY` to set the rpc type of ratis to netty, the default is grpc.

Review Comment:
   ```suggestion
   
   For example: ues `-Draft.rpc.type=NETTY` to set the RPC type of Ratis to Netty, the default is gRPC.
   ```



-- 
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@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] onebox-li commented on a diff in pull request #1530: [CELEBORN-623] Ratis-shell add default rpc type

Posted by "onebox-li (via GitHub)" <gi...@apache.org>.
onebox-li commented on code in PR #1530:
URL: https://github.com/apache/incubator-celeborn/pull/1530#discussion_r1214293180


##########
docs/celeborn_ratis_shell.md:
##########
@@ -65,7 +65,8 @@ Usage: celeborn-ratis sh [generic options]
 ## generic options
 The `generic options` pass values for a given ratis-shell property.
 It support the following content:
-`-D*`, `-X*`, `-agentlib*`, `-javaagent*`
+`-D*`, `-X*`, `-agentlib*`, `-javaagent*`  

Review Comment:
   yes, two spaces for a new line



-- 
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@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] pan3793 commented on pull request #1530: [CELEBORN-623][DOCS] Document how to change RPC type in `celeborn-ratis`

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #1530:
URL: https://github.com/apache/incubator-celeborn/pull/1530#issuecomment-1573654389

   @onebox-li thanks for your contribution, BTW, you may need to update your GitHub profile to link your commit mail `liyihe <li...@bigo.sg>`, otherwise your contributions will not be counted by GitHub.


-- 
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@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] onebox-li commented on a diff in pull request #1530: [CELEBORN-623] Ratis-shell add default rpc type

Posted by "onebox-li (via GitHub)" <gi...@apache.org>.
onebox-li commented on code in PR #1530:
URL: https://github.com/apache/incubator-celeborn/pull/1530#discussion_r1214297283


##########
docs/celeborn_ratis_shell.md:
##########
@@ -65,7 +65,8 @@ Usage: celeborn-ratis sh [generic options]
 ## generic options
 The `generic options` pass values for a given ratis-shell property.
 It support the following content:
-`-D*`, `-X*`, `-agentlib*`, `-javaagent*`
+`-D*`, `-X*`, `-agentlib*`, `-javaagent*`  

Review Comment:
   ok



-- 
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@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] FMX commented on a diff in pull request #1530: [CELEBORN-623] Ratis-shell add default rpc type

Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on code in PR #1530:
URL: https://github.com/apache/incubator-celeborn/pull/1530#discussion_r1213870927


##########
bin/celeborn-ratis:
##########
@@ -71,6 +71,8 @@ function main {
   CELEBORN_RATIS_SHELL_JAVA_OPTS+=" -Dorg.apache.jasper.compiler.disablejsr199=true"
   CELEBORN_RATIS_SHELL_JAVA_OPTS+=" -Djava.net.preferIPv4Stack=true"
   CELEBORN_RATIS_SHELL_JAVA_OPTS+=" -Dorg.apache.ratis.thirdparty.io.netty.allocator.useCacheForAllThreads=false"
+  # Netty is supported by default
+  CELEBORN_RATIS_SHELL_JAVA_OPTS+=" -Draft.rpc.type=NETTY"

Review Comment:
   > some background:
   > 
   > * `netty` is chosen by coincidence, based on our research, `grpc` is selected by most of the projects that use ratis, maybe we need to investigate if we can switch to `grpc` at some time
   > * the ratis shell support changing rpc type by args
   
   According to the latest test, using `grpc` is runnable but with higher election time than netty. There are no clear benefits to using `grpc`.



-- 
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@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1530: [CELEBORN-623] Ratis-shell add default rpc type

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1530:
URL: https://github.com/apache/incubator-celeborn/pull/1530#discussion_r1214295850


##########
docs/celeborn_ratis_shell.md:
##########
@@ -65,7 +65,8 @@ Usage: celeborn-ratis sh [generic options]
 ## generic options
 The `generic options` pass values for a given ratis-shell property.
 It support the following content:
-`-D*`, `-X*`, `-agentlib*`, `-javaagent*`
+`-D*`, `-X*`, `-agentlib*`, `-javaagent*`  

Review Comment:
   Emm ... how about moving the example after the code expression?



-- 
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@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] pan3793 commented on pull request #1530: [CELEBORN-623] Ratis-shell add default rpc type

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #1530:
URL: https://github.com/apache/incubator-celeborn/pull/1530#issuecomment-1573647410

   Thanks, merging to main/0.3


-- 
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@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] pan3793 closed pull request #1530: [CELEBORN-623][DOCS] Document how to change RPC type in `celeborn-ratis`

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 closed pull request #1530: [CELEBORN-623][DOCS] Document how to change RPC type in `celeborn-ratis`
URL: https://github.com/apache/incubator-celeborn/pull/1530


-- 
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@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1530: [CELEBORN-623] Ratis-shell add default rpc type

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1530:
URL: https://github.com/apache/incubator-celeborn/pull/1530#discussion_r1214285148


##########
docs/celeborn_ratis_shell.md:
##########
@@ -65,7 +65,8 @@ Usage: celeborn-ratis sh [generic options]
 ## generic options
 The `generic options` pass values for a given ratis-shell property.
 It support the following content:
-`-D*`, `-X*`, `-agentlib*`, `-javaagent*`
+`-D*`, `-X*`, `-agentlib*`, `-javaagent*`  

Review Comment:
   extra spaces?



-- 
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@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1530: [CELEBORN-623] Ratis-shell add default rpc type

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1530:
URL: https://github.com/apache/incubator-celeborn/pull/1530#discussion_r1214298463


##########
docs/celeborn_ratis_shell.md:
##########
@@ -65,7 +65,8 @@ Usage: celeborn-ratis sh [generic options]
 ## generic options
 The `generic options` pass values for a given ratis-shell property.
 It support the following content:
-`-D*`, `-X*`, `-agentlib*`, `-javaagent*`
+`-D*`, `-X*`, `-agentlib*`, `-javaagent*`  
+For example: Using `-Draft.rpc.type=NETTY` to set the rpc type of ratis to netty, the default is grpc.

Review Comment:
   Using => use
   
   the docs sentence should be imperative, present tense



-- 
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@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] AngersZhuuuu commented on pull request #1530: [CELEBORN-623] Ratis-shell add default rpc type

Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on PR #1530:
URL: https://github.com/apache/incubator-celeborn/pull/1530#issuecomment-1571259426

   I remember ratis shell can add config to choose netty pr grpc?


-- 
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@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1530: [CELEBORN-623] Ratis-shell add default rpc type

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1530:
URL: https://github.com/apache/incubator-celeborn/pull/1530#discussion_r1212521956


##########
bin/celeborn-ratis:
##########
@@ -71,6 +71,8 @@ function main {
   CELEBORN_RATIS_SHELL_JAVA_OPTS+=" -Dorg.apache.jasper.compiler.disablejsr199=true"
   CELEBORN_RATIS_SHELL_JAVA_OPTS+=" -Djava.net.preferIPv4Stack=true"
   CELEBORN_RATIS_SHELL_JAVA_OPTS+=" -Dorg.apache.ratis.thirdparty.io.netty.allocator.useCacheForAllThreads=false"
+  # Netty is supported by default
+  CELEBORN_RATIS_SHELL_JAVA_OPTS+=" -Draft.rpc.type=NETTY"

Review Comment:
   some background:
   
   - `netty` is chosen by coincidence, based on our research, `grpc` is selected by most of the projects that use ratis, maybe we need to investigate if we can switch to `grpc` at some time
   - the ratis shell support changing rpc type by args



-- 
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@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org