-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-37925][DOC] Update document to mention the workaround for YARN-11053 #35223
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
|
Could you also include the screen-capture for generated web documentation in the PR description if possible ?? |
|
@itholic updated. |
|
Can one of the admins verify this patch? |
|
@itholic could you please take a look again? |
|
@xkrogen Could you happen to take a look at this since you initially writing the "Running multiple versions of the Spark Shuffle Service" section ?? (#31936) also cc @tgravescs @dongjoon-hyun as a reviewer of the previous PR, FYI. |
xkrogen
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 filing this @pan3793 !
It seems that this was broken by YARN-9075, which means it is only broken for 3.3.0 and 3.3.1. Given that it is only broken for such a narrow subset of versions, maybe we can move the workaround instructions to a separate part of the documentation (e.g. a separate section lower down), so it doesn't clutter up the main documentation for this feature?
docs/running-on-yarn.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.
Rather than setting it to some arbitrary class name, it would be better to set it to an empty string. In this way, the ApplicationClassLoader will be configured to use the default system classes (ref). This is probably more desirable behavior than completely overriding the system classes config, and is more similar to the changes from YARN-11053.
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.
Let's not add this as it is a workaround for older YARN. It does not need to be a part of Spark docs
docs/running-on-yarn.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.
Let's clean up the sentence a little:
"Note that versions of YARN prior to 3.3.2/3.4.0 have an issue[...] which requires setting yarn.nodemanager.aux-services.<service-name>.system-classes as a workaround. See issue for details."
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.
does this affect all 2.9.0 til 3.3.2? it would be good to be explicit about the versions it affects.
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.
As I mentioned above, this should only affect versions 3.3.0 and 3.3.1
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 comments, updated.
docs/running-on-yarn.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.
Let's not add this as it is a workaround for older YARN. It does not need to be a part of Spark docs
…ARN-11053" This reverts commit d60d899f37eca27b4653d204a31d4770babdef82.
|
Updated based on comments, would you please take a look again? @srowen |
docs/running-on-yarn.md
Outdated
| this. In addition to setting up separate classpaths, it's necessary to ensure the two versions | ||
| advertise to different ports. This can be achieved using the `spark-shuffle-site.xml` file described | ||
| above. For example, you may have configuration like: | ||
| this. Notes that YARN 3.3.0/3.3.1 have an issue which requires setting |
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.
Notes -> Note
as a workaround, see -> as a workaround. See
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.
updated.
|
Merged to master |
What changes were proposed in this pull request?
Update document "Running multiple versions of the Spark Shuffle Service" to mention the workaround for YARN-11053
Why are the changes needed?
User may stuck when they following the current document to deploy multi-versions Spark Shuffle Service on YARN because of YARN-11053
Does this PR introduce any user-facing change?
User document changes.
How was this patch tested?