-
Notifications
You must be signed in to change notification settings - Fork 117
Config for hard cpu limit on pods; default unlimited #356
Config for hard cpu limit on pods; default unlimited #356
Conversation
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.
@sandflee can you please change this PR so that if spark.kubernetes.executor.limit.cores is not set then there is no .addToLimits("cpu", executorCpuLimitQuantity) call here?
From discussion on the linked issue I think we want the default to be no cpu limit, and only add a limit if the user specifies one
|
Linking to #38 which is the umbrella issue for fully-generic customization of things like this via user-provided yaml files instead of incrementally adding additional options for every k8s feature. |
|
patch updated, add spark.kubernetes.driver.limit.cores besides spark.kubernetes.executor.limit.cores |
ash211
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.
A couple tiny changes and then this LGTM. Let's merge once you fix those bits.
@mccheah any thoughts?
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 should emphasize that this is a single executor, like hard cpu limit for a single executor
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.
drop the .getOrElse("") and leave this value as Option[String]
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.
leave as Option[String]
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.
de-indent a bit
docs/running-on-kubernetes.md
Outdated
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.
Looks rest of the docs for config keys all use capitalized full sentences. Can you do the same to be consistent?
docs/running-on-kubernetes.md
Outdated
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.
Ditto.
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.
Ditto.
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.
Ditto.
|
rerun integration test please |
|
Hi @sandflee it looks like a recent commit caused a merge conflict in this PR. Can you please fix that and address the outstanding comments? With those minor changes I think this PR is ready to merge |
|
Thank a bunch @sandflee for the contribution! Looking forward to jobs feeling faster for everyone |
#352