You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "waitinfuture (via GitHub)" <gi...@apache.org> on 2023/06/10 08:35:53 UTC
[GitHub] [incubator-celeborn] waitinfuture opened a new pull request, #1574: [CELEBORN-661] Try avoid local blacklist when offer slots
waitinfuture opened a new pull request, #1574:
URL: https://github.com/apache/incubator-celeborn/pull/1574
<!--
Thanks for sending a pull request! Here are some tips for you:
- Make sure the PR title start w/ a JIRA ticket, e.g. '[CELEBORN-XXXX] Your PR title ...'.
- Be sure to keep the PR description updated to reflect all changes.
- Please write your PR title to summarize what this PR proposes.
- If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
LifecycleManager passes local blacklist in ```RequestSlots```, master will try to avoid the workers in the blacklist when offer slots, and ignore the blacklist if offer slots fails.
### Why are the changes needed?
I was testing the case of three workers with replication on, and try to kill then start a worker without graceful shutdown, since master only considers a worker as lost after ```WORKER_HEARTBEAT_TIMEOUT```, which is default 120s, master will blindly assign slots to the dead worker, which causes further more revivings.
Before this PR, test time is ```245s```, after this PR, time is ```230s```.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Manual test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] waitinfuture commented on pull request #1574: [CELEBORN-661] Try avoid local blacklist when offer slots
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #1574:
URL: https://github.com/apache/incubator-celeborn/pull/1574#issuecomment-1585565207
cc @RexXiong @AngersZhuuuu @pan3793
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1574: [CELEBORN-661] Try avoid local blacklist when offer slots
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1574:
URL: https://github.com/apache/incubator-celeborn/pull/1574#discussion_r1225412454
##########
common/src/main/proto/TransportMessages.proto:
##########
@@ -183,6 +183,7 @@ message PbRequestSlots {
int32 storageType = 7;
PbUserIdentifier userIdentifier = 8;
bool shouldRackAware = 9;
+ repeated PbWorkerInfo localBlacklist = 10;
Review Comment:
maybe `clientSideExcludedWorker`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] waitinfuture commented on pull request #1574: [CELEBORN-661] Try avoid local blacklist when offer slots
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #1574:
URL: https://github.com/apache/incubator-celeborn/pull/1574#issuecomment-1585618171
@RexXiong I've added config and refine the logic to ensure the at most 1/3 workers are filtered out, please take a look again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] github-actions[bot] commented on pull request #1574: [CELEBORN-661] Try avoid local blacklist when offer slots
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1574:
URL: https://github.com/apache/incubator-celeborn/pull/1574#issuecomment-1616449688
This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on a diff in pull request #1574: [CELEBORN-661] Try avoid local blacklist when offer slots
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #1574:
URL: https://github.com/apache/incubator-celeborn/pull/1574#discussion_r1225412454
##########
common/src/main/proto/TransportMessages.proto:
##########
@@ -183,6 +183,7 @@ message PbRequestSlots {
int32 storageType = 7;
PbUserIdentifier userIdentifier = 8;
bool shouldRackAware = 9;
+ repeated PbWorkerInfo localBlacklist = 10;
Review Comment:
maybe `clientExcludedWorker`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on pull request #1574: [CELEBORN-661] Try avoid local blacklist when offer slots
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #1574:
URL: https://github.com/apache/incubator-celeborn/pull/1574#issuecomment-1585703701
Thanks for introducing this interesting design.
Some thoughts on the design aspect. This switch can either be a client-side or a master-side configuration.
### client-side
#### pros
- can be enabled/disabled in application granularity
- when disabled, reduce the control message size
#### cons
- ugly design, because it actually is part of a slot assignment algorithm
### master-side
#### pros
- unified design, centralized the logic of slot assignment on the master side
#### cons
- can only be enabled/disabled in the cluster granularity
- client also should send `clientExcludedWork` in the control message
Personally, I lean to make it a master-side configuration. Because I don't think we should disable it, and the control message fluid increasing is acceptable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] pan3793 commented on pull request #1574: [CELEBORN-661] Try avoid local blacklist when offer slots
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #1574:
URL: https://github.com/apache/incubator-celeborn/pull/1574#issuecomment-1586083286
If make it a client-side configuration, maybe the name can be
```
celeborn.client.registerShuffle.reportClientExcludedWorkers.enabled
```
"report" should be accurate than "avoid", because the decision is actually made by the master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] waitinfuture commented on pull request #1574: [CELEBORN-661] Try avoid local blacklist when offer slots
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #1574:
URL: https://github.com/apache/incubator-celeborn/pull/1574#issuecomment-1586057913
>
Hi @pan3793 , thanks for your feedback on the design!
Here is my thought. When make it a client-side config, it's like ```master will always consider the client-excluded workers, it's client's decision whether to send them```. On the other hand, when make it a master-side config, it's like ```client will always send locally-excluded workers, it's master's decision whether to consider them```. So IMHO, in the aspect of design unification, theses two seems at the same level.
In addition, https://github.com/apache/incubator-celeborn/pull/1575 reports worker unavailable as soon as it's closed(not for kill -9 for sure), so the delay of worker-lost to master has improved, and perhaps it's not that necessary to always turn this switch on.
Any further thoughts please let me know, thanks!
cc @RexXiong
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] github-actions[bot] commented on pull request #1574: [CELEBORN-661] Try avoid local blacklist when offer slots
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1574:
URL: https://github.com/apache/incubator-celeborn/pull/1574#issuecomment-1632081082
This issue was closed because it has been staled for 10 days with no activity.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] codecov[bot] commented on pull request #1574: [CELEBORN-661] Try avoid local blacklist when offer slots
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #1574:
URL: https://github.com/apache/incubator-celeborn/pull/1574#issuecomment-1585567556
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1574?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#1574](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1574?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (139055d) into [main](https://app.codecov.io/gh/apache/incubator-celeborn/commit/6b725202a2526b70e7eddbc9f9c0b15ee6ea6489?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (6b72520) will **increase** coverage by `0.01%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## main #1574 +/- ##
==========================================
+ Coverage 45.41% 45.41% +0.01%
==========================================
Files 158 158
Lines 9830 9836 +6
Branches 968 968
==========================================
+ Hits 4463 4466 +3
- Misses 5066 5069 +3
Partials 301 301
```
| [Impacted Files](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1574?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...born/common/protocol/message/ControlMessages.scala](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1574?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL3Byb3RvY29sL21lc3NhZ2UvQ29udHJvbE1lc3NhZ2VzLnNjYWxh) | `1.50% <0.00%> (-0.01%)` | :arrow_down: |
... and [1 file with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1574/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] RexXiong commented on pull request #1574: [CELEBORN-661] Try avoid local blacklist when offer slots
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on PR #1574:
URL: https://github.com/apache/incubator-celeborn/pull/1574#issuecomment-1586468876
The gap here is that the master cannot know the unavailability of the worker in time, but the lifecycleManager can. I prefer configuration on the master side or both have. For the highest efficiency of the overall cluster the master needs to know the status of the workers from various channels, and this is can be considered as a strategy assigned by the master (whether to consider excluding workers on the client side, and besides the client blacklist also has a delay), as the current job needs to exclude this worker, can it be extended to another newly launched job?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-celeborn] github-actions[bot] closed pull request #1574: [CELEBORN-661] Try avoid local blacklist when offer slots
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #1574: [CELEBORN-661] Try avoid local blacklist when offer slots
URL: https://github.com/apache/incubator-celeborn/pull/1574
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org