-
Notifications
You must be signed in to change notification settings - Fork 13.9k
[FLINK-12139][Mesos] Add disk space parameter. #8224
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
|
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. DetailsThe Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
tillrohrmann
left a comment
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.
Thanks for this fix @juangentile. My only question is what the default value of Mesos is for the disk value if it is not specified. Before we did not specify it so we should make sure that this is the default value of MESOS_RM_TASKS_DISK_MB.
Did you test your changes on a Mesos cluster (w/ and w/o setting the disk size)?
| taskInfo.addAllResources(allocation.takeScalar("cpus", taskRequest.getCPUs(), roles)); | ||
| taskInfo.addAllResources(allocation.takeScalar("gpus", taskRequest.getGPUs(), roles)); | ||
| taskInfo.addAllResources(allocation.takeScalar("mem", taskRequest.getMemory(), roles)); | ||
| taskInfo.addAllResources(allocation.takeScalar("disk", taskRequest.getDisk(), roles)); |
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.
What is the default Mesos value if disk is not specified?
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.
Hello Till,
Sorry for the delay in the response.
I tested flink on the mesos cluster we have at Criteo and before my change it didn't work because it was taking the default 0 value for the disk limit.
In our cluster we enforce the disk usage check but setting this parameter --enforce_container_disk_quota.
You can find the description of the parameter here: http://mesos.apache.org/documentation/latest/configuration/agent/
(and here http://mesos.apache.org/documentation/latest/configuration/agent/ - http://mesos.apache.org/documentation/latest/isolators/disk-du/)
The default installation of mesos has false for enforce_container_disk_quota so the disk limit is never checked.
After my change, it works correctly on our mesos cluster (if we set a value different from the default 0 value using the new parameter).
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.
Hmm, looking at the Mesos source code it seems that the disk size will be automatically calculated if not specified. Thus, I would suggest to only set the "disk" resource value if the value is larger than 0.0.
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.
sure, I'll do the change :)
tillrohrmann
left a comment
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.
Thanks for commenting on my comment @juangentile. I think to be on the safe side we could only set the disk value if the value is different than 0.0.
| taskInfo.addAllResources(allocation.takeScalar("cpus", taskRequest.getCPUs(), roles)); | ||
| taskInfo.addAllResources(allocation.takeScalar("gpus", taskRequest.getGPUs(), roles)); | ||
| taskInfo.addAllResources(allocation.takeScalar("mem", taskRequest.getMemory(), roles)); | ||
| taskInfo.addAllResources(allocation.takeScalar("disk", taskRequest.getDisk(), roles)); |
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.
Hmm, looking at the Mesos source code it seems that the disk size will be automatically calculated if not specified. Thus, I would suggest to only set the "disk" resource value if the value is larger than 0.0.
tillrohrmann
left a comment
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.
Thanks for addressing my comments @juangentile. LGTM. Merging this PR.
What is the purpose of the change
Add a parameter to set the required disk space for a Mesos deployment. Before this change the default value is 0 with no option to change it. We maintain the default 0, we only make it parameterizable
Brief change log
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation