-
Notifications
You must be signed in to change notification settings - Fork 28.1k
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
[MINOR][DOCS] Fix a typo in ContainerPlacementStrategy's class comment #28267
Conversation
The change looks good but mind taking another look and see if there are some more typos around here? I am sure there are some more typos to fix. |
OK, will proofread again later today |
@HyukjinKwon read again and fixed several more words. |
ok to test |
@@ -40,7 +40,7 @@ private[yarn] case class ContainerLocalityPreferences(nodes: Array[String], rack | |||
* and cpus per task is 1, so the required container number is 15, | |||
* and host ratio is (host1: 30, host2: 30, host3: 20, host4: 10). | |||
* | |||
* 1. If requested container number (18) is more than the required container number (15): | |||
* 1. If the requested container number (18) is more than the required container number (15): |
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.
I think we can remove this the
* and the expected containers on each node would be (host1: 5, host2: 5, host3: 4, host4: 2), | ||
* so the newly requested containers on each node would be updated to (host1: 4, host2: 4, | ||
* host3: 3, host4: 1), 12 containers by total. | ||
* | ||
* 4.1 If requested container number (18) is more than newly required containers (12). Follow | ||
* method 1 with updated ratio 4 : 4 : 3 : 1. | ||
* method 1 with an updated ratio 4 : 4 : 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.
I think this an
can be removed too.
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 checking LGTM except a couple of comments.
ok to test |
Test build #121563 has finished for PR 28267 at commit
|
Test build #121564 has finished for PR 28267 at commit
|
Close enough - merged to master/3.0 |
### What changes were proposed in this pull request? This PR fixes a typo in deploy/yarn/LocalityPreferredContainerPlacementStrategy.scala file. ### Why are the changes needed? To deliver correct explanation about how the placement policy works. ### Does this PR introduce any user-facing change? No ### How was this patch tested? UT as specified, although shouldn't influence any functionality since it's in the comment. Closes #28267 from asclepiusaka/master. Authored-by: Cong Du <asclepius1993@gmail.com> Signed-off-by: Sean Owen <srowen@gmail.com> (cherry picked from commit 54b97b2) Signed-off-by: Sean Owen <srowen@gmail.com>
What changes were proposed in this pull request?
This PR fixes a typo in deploy/yarn/LocalityPreferredContainerPlacementStrategy.scala file.
Why are the changes needed?
To deliver correct explanation about how the placement policy works.
Does this PR introduce any user-facing change?
No
How was this patch tested?
UT as specified, although shouldn't influence any functionality since it's in the comment.