Skip to content

Conversation

@guoxiaolongzte
Copy link

What changes were proposed in this pull request?

The description of "this requires spark.shuffle.service.enabled to be set" for the spark.dynamicAllocation.enabled configuration item is not clear. I am not sure how to set spark.shuffle.service.enabled is true or false, so that the user to guess, resulting in doubts. All i have changed here, stressed that must spark.shuffle.service.enabled to be set true.

When I set spark.dynamicAllocation.enabled=true, but set spark.shuffle.service.enabled false, When I submit the spark-submit --master yarn job, the program throws the exception.

How was this patch tested?

manual tests

Please review http://spark.apache.org/contributing.html before opening a pull request.

….enabled to be set" for the spark.dynamicAllocation.enabled configuration item is not clear. I am not sure how to set spark.shuffle.service.enabled is true or false, so that the user to guess, resulting in doubts. All i have changed here, stressed that must spark.shuffle.service.enabled to be set true.
@AmplabJenkins
Copy link

Can one of the admins verify this patch?

<a href="job-scheduling.html#dynamic-resource-allocation">here</a>.
<br><br>
This requires <code>spark.shuffle.service.enabled</code> to be set.
This requires <code>spark.shuffle.service.enabled</code> to be set true.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's no ambiguity here. Usually configuration with name "xxx.enabled" can only have two values "true" or "false". So "to be set" usually means to enable it (to set it to true).

Copy link
Author

Choose a reason for hiding this comment

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

You are right, but usually is not very sure. Can not let the user to guess, the document needs to be accurately described. In addition, the spark project, several other are clearly described, spark.shuffle.service.enabled set true.
1
2

Copy link
Contributor

Choose a reason for hiding this comment

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

Since other places are clearly defined the property, so there should be no ambiguity. Personally I'm not fond of this super nit fix...

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your comments.
This requires spark.shuffle.service.enabled to be set true. It is very clearly. Only such an accurate description,there be no ambiguity.

@guoxiaolongzte
Copy link
Author

@srowen Help review the code,Thanks.

@guoxiaolongzte guoxiaolongzte changed the title [SPARK-21600] The description of "this requires spark.shuffle.service.enabled to be set" for the spark.dynamicAllocation.enabled configuration item is not clear [SPARK-21600][docs] The description of "this requires spark.shuffle.service.enabled to be set" for the spark.dynamicAllocation.enabled configuration item is not clear Aug 2, 2017
@guoxiaolongzte
Copy link
Author

@srowen Help review the code,Thanks.

@srowen
Copy link
Member

srowen commented Aug 11, 2017

"set" is pretty synonymous with "true" for boolean properties. Its name includes 'enabled'. I think this is too trivial.

srowen added a commit to srowen/spark that referenced this pull request Sep 12, 2017
@srowen srowen mentioned this pull request Sep 12, 2017
@asfgit asfgit closed this in dd88fa3 Sep 13, 2017
zifeif2 pushed a commit to zifeif2/spark that referenced this pull request Nov 22, 2025
Closes apache#18522
Closes apache#17722
Closes apache#18879
Closes apache#18891
Closes apache#18806
Closes apache#18948
Closes apache#18949
Closes apache#19070
Closes apache#19039
Closes apache#19142
Closes apache#18515
Closes apache#19154
Closes apache#19162
Closes apache#19187
Closes apache#19091

Author: Sean Owen <[email protected]>

Closes apache#19203 from srowen/CloseStalePRs3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants