HDFS-17545. [ARR] router async rpc client. #6871
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hello everyone, this PR needs to use Asyncutil of HDFS-17543 and async ProtocolPB of HDFS-17544, We should first review HDFS-17543 and HDFS-17544 and then review this PR. Before HDFS-17543、HDFS-17544 is merged, this PR is only used as an example of subsequent use of AsyncUtil、 async ProtocolPB. |
| if (asyncRouterResponder == null) { | ||
| LOG.info("init router async responder count: {}", asyncResponderCount); | ||
| asyncRouterResponder = Executors.newFixedThreadPool( | ||
| asyncHandlerCount, new AsyncThreadFactory("router async responder ")); |
There was a problem hiding this comment.
@KeeProMise Hi, sir. here should be asyncResponderCount right?
There was a problem hiding this comment.
@hfutatzhanghb Yes, I will change this to asyncResponderCount later. Thank you for your careful review.
| try { | ||
| return ApplyFunction.this.apply(t); | ||
| } catch (IOException e) { | ||
| throw new CompletionException(e); |
There was a problem hiding this comment.
@KeeProMise Hi, sir. It seems that this CompletionException isn't propagated to upper caller methods. Please check whether should we catch this exception or not ? Thanks.
There was a problem hiding this comment.
@hfutatzhanghb Hi, CompletionException is passed to the returned CompletableFuture object. The CompletionException exception in the returned CompletableFuture can be processed later through CatchFunction or AsyncCatchFunction. Therefore, no additional processing is required here.
| @@ -0,0 +1,26 @@ | |||
| package org.apache.hadoop.hdfs.server.federation.router; | |||
There was a problem hiding this comment.
@KeeProMise Sir, we should add license header here.
There was a problem hiding this comment.
@hfutatzhanghb Thank you for your careful review, i will add this later!
…easier. (apache#6868). Contributed by Jian Zhang. Reviewed-by: hfutatzhanghb <[email protected]> Signed-off-by: He Xiaoqiao <[email protected]>
| @@ -284,6 +304,12 @@ public RouterRpcServer(Configuration conf, Router router, | |||
| int handlerQueueSize = this.conf.getInt(DFS_ROUTER_HANDLER_QUEUE_SIZE_KEY, | |||
| DFS_ROUTER_HANDLER_QUEUE_SIZE_DEFAULT); | |||
|
|
|||
| this.enableAsync = conf.getBoolean(DFS_ROUTER_RPC_ENABLE_ASYNC, | |||
| true); | |||
There was a problem hiding this comment.
@KeeProMise Sir, a minor opinion. use default value here.
There was a problem hiding this comment.
@hfutatzhanghb Thank you for your careful review, done!
|
💔 -1 overall
This message was automatically generated. |
…ny. (apache#6870). Contributed by Jian Zhang. Signed-off-by: He Xiaoqiao <[email protected]>
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@goiri @simbadzina @Hexiaoqiao @sjlee @ayushtkn @haiyang1987 @ZanderXu |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Thanks Invite me, I will take some time to review this PR. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Hi, @goiri @simbadzina @Hexiaoqiao @sjlee @ayushtkn @haiyang1987 @ZanderXu @yuanboliu the PR has been blocked for 2 months, and if everyone has time, please help to review it because several subtask PRs need to depend on this PR. This PR does not modify the existing synchronous router logic. To facilitate everyone's understanding and review of this PR, my main modifications are as follows:
IMO, this PR does not modify any existing code or logic; it simply adds asynchronous functionality (which is supported by a toggle configuration) and adjusts the code structure, without any adverse impact on existing features. |
Hexiaoqiao
force-pushed
the
HDFS-17531
branch
from
4e0b405 to
a591902
Compare
# Conflicts: # hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/protocolPB/AsyncRpcProtocolPBUtil.java # hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/async/AsyncUtil.java # hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/protocolPB/TestAsyncRpcProtocolPBUtil.java # hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/protocolPB/TestRouterClientSideTranslatorPB.java
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Hexiaoqiao
approved these changes
Sep 27, 2024
|
@KeeProMise Thanks for your works. LGTM. I think it is ready to commit to branch-17531 and let's continue the rest PR. Thanks again. |
@Hexiaoqiao @hfutatzhanghb Thanks for your review! |
KeeProMise
added a commit
to KeeProMise/hadoop
that referenced
this pull request
Nov 21, 2024
… by Jian Zhang. Reviewed-by: hfutatzhanghb <[email protected]> Signed-off-by: He Xiaoqiao <[email protected]>
KeeProMise
added a commit
to KeeProMise/hadoop
that referenced
this pull request
Nov 21, 2024
… by Jian Zhang. Reviewed-by: hfutatzhanghb <[email protected]> Signed-off-by: He Xiaoqiao <[email protected]>
KeeProMise
added a commit
to hfutatzhanghb/hadoop
that referenced
this pull request
Dec 26, 2024
… by Jian Zhang. Reviewed-by: hfutatzhanghb <[email protected]> Signed-off-by: He Xiaoqiao <[email protected]>
KeeProMise
added a commit
to KeeProMise/hadoop
that referenced
this pull request
Jan 17, 2025
… by Jian Zhang. Reviewed-by: hfutatzhanghb <[email protected]> Signed-off-by: He Xiaoqiao <[email protected]>
KeeProMise
added a commit
to KeeProMise/hadoop
that referenced
this pull request
Jan 20, 2025
… by Jian Zhang. Reviewed-by: hfutatzhanghb <[email protected]> Signed-off-by: He Xiaoqiao <[email protected]>
4 tasks
KeeProMise
added a commit
to KeeProMise/hadoop
that referenced
this pull request
Feb 11, 2025
… by Jian Zhang. Reviewed-by: hfutatzhanghb <[email protected]> Signed-off-by: He Xiaoqiao <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
please see: https://issues.apache.org/jira/browse/HDFS-17545
NOTE: This is a sub-pull request (PR) related to HDFS-17531(Asynchronous router RPC). For more details or context, please refer to the main issue HDFS-17531
More detailed documentation: HDFS-17531 Router asynchronous rpc implementation.pdf and Aynchronous router.pdf
Main modifications:
How was this patch tested?
new UT TestRouterAsyncRpcClient
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?