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-2368 Making FileServerHandler sends gracefully to prevent OOM #1303
Conversation
Busy wait for the channel to be ready before writing to it to prevent the buffer from exploding. Also making some small code changes for clarity.
Merged build triggered. |
Merged build started. |
while (!channel.isWritable()) { | ||
channel.writeAndFlush(object); | ||
} | ||
} |
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.
This method is incorrect - if channel was writable initially, it will exit immediately without any write.
Has this patch been tested ?
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.
good catch! Will test it.
The config changes look promising (btw, you will need to read them from SparkConf and not hardcode values) - rest can be ommitted. |
Merged build finished. |
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16345/ |
Merged build triggered. |
Merged build started. |
Merged build finished. All automated tests passed. |
All automated tests passed. |
+ plus documentation
Merged build triggered. |
Merged build started. |
Merged build finished. All automated tests passed. |
All automated tests passed. |
@mridulm please take another look. Thanks |
Hi @ngbinh, thanks for your PR. It appears that we are exposing a lot of configs to the user, and this might be confusing if there are all these extra knobs that we can tune. I think it's ok to keep the configs, but I wonder whether we should actually document them. Also, many of these configs are under the Can you up merge this once you have the chance? |
@andrewor14 -- Reynold has changed a lot of these files as a part of the shuffle refactoring (In fact I think most of these java files don't exist anymore). @rxin Could you incorporate these options as a part of your netty changes ? |
I agree that these changes look outdated now. I believe Akka-http streaming http://typesafe.com/blog/typesafe-announces-akka-streams is the future. I remember seeing a ticket where @rxin listed a number of improvements on Spark network code that should cover these changes. Close for now. |
Busy wait for the channel to be ready before writing to it to prevent
the buffer from exploding.
Also making some small code changes to improve clarity.