-
Notifications
You must be signed in to change notification settings - Fork 161
[#2537] feat(spark): Introduce option to activate small cache in grpc server #2538
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
Conversation
@rickyma Could you take a look at this PR? |
.booleanType() | ||
.defaultValue(false) | ||
.withDescription( | ||
"The option to control whether enable the pooled byte buf allocator small cache. This is only valid for spark driver side grpc server"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Option to control whether to enable the small cache in the pooled byte buffer allocator. This option is only applicable to the gRPC server on the Spark driver side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we need to describe this new config in docs.
@@ -102,14 +103,17 @@ static void reset() { | |||
} | |||
|
|||
private Server buildGrpcServer(int serverPort) { | |||
boolean isClientSmallCacheEnabled = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can put this config into RssBaseConf
also? Because all other configs are in it.
In this way, we should rename it to isSmallCacheEnabled
thus it could be used both in clients and servers?
Then, we need to change the description of it as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. This option could be extended to cover the #1780 requirements.
PTAL @rickyma |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM
What changes were proposed in this pull request?
Introduce the config option to activate small cache in grpc server
Why are the changes needed?
for #2537
When partition reassignment is enabled in the production environment, we observed that some Spark jobs failed due to gRPC request timeouts (DEADLINE_EXCEEDED). Upon investigating the Spark driver logs, we found severe GC events, indicating significant memory pressure on the driver process.
Based on the PR #1780, the small cache looks effective for the grpc mode.
This PR is to make the small cache being enabled as the default option because GRPC_NETTY mode has been as the default rpc mode.
Does this PR introduce any user-facing change?
Yes.
rss.rpc.netty.smallCacheEnabled=true
How was this patch tested?
Existing unit tests.