Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-21701][CORE] Enable RPC client to use SO_RCVBUF, SO_SNDBUF and SO_BACKLOG in SparkConf #18922

Closed
wants to merge 1 commit into from

Conversation

neoremind
Copy link

What changes were proposed in this pull request?

  1. TCP parameters like SO_RCVBUF, SO_SNDBUF and SO_BACKLOG can be set in SparkConf, and org.apache.spark.network.server.TransportServer can use those parameters to build server by leveraging netty. But for TransportClientFactory, there is no such way to set those parameters from SparkConf. This could be inconsistent in server and client side when people set parameters in SparkConf. So this PR make RPC client to be enable to use those TCP parameters as well.

  2. Add some comments to MessageDecoder class to show how wire format looks like for a RPC message and draw a vivid graph to present to detailed wire protocol.

How was this patch tested?

Existing tests and refine asking non-existent endpoint test case in RpcEnvSuite.

…KLOG` in SparkConf.

2.	Add Javadoc comment in MessageEncoder class.
3.	Refine test case of asking non-existent endpoint in RPC test suite.
@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@neoremind
Copy link
Author

@zsxwing Would you mind verifying the patch for me? I notice you have contributed to rpc module in spark. Many thanks!

@jerryshao
Copy link
Contributor

@neoremind can we separate different issues into different PRs? I saw you add several different fixes other than the PR title described.

@@ -210,6 +210,18 @@ private TransportClient createClient(InetSocketAddress address)
.option(ChannelOption.CONNECT_TIMEOUT_MILLIS, conf.connectionTimeoutMs())
.option(ChannelOption.ALLOCATOR, pooledAllocator);

if (conf.backLog() > 0) {
bootstrap.option(ChannelOption.SO_BACKLOG, conf.backLog());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is really used for Client bootstrap? AFAIK backlog is mainly used in server side to cache the incoming connections.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your thoughts. I will eliminate the setting of the param.

@neoremind
Copy link
Author

@jerryshao Thanks for your reviewing! Per your advice, I have separated the issue into different PRs,
#18964
and
#18965

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants