You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "AngersZhuuuu (via GitHub)" <gi...@apache.org> on 2023/06/13 07:17:12 UTC
[GitHub] [incubator-celeborn] AngersZhuuuu opened a new pull request, #1584: [CELEBORN-668][FOLLOWUP] Handle unknown worker should also reply WorkerLostResponse
AngersZhuuuu opened a new pull request, #1584:
URL: https://github.com/apache/incubator-celeborn/pull/1584
### What changes were proposed in this pull request?
Now worker will send WorkLost too, master should also reply WorkerLostResponse when it's unknown worker
### Why are the changes needed?
### Does this PR introduce _any_ user-facing change?
### How was this patch tested?
--
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] AngersZhuuuu commented on a diff in pull request #1584: [CELEBORN-668][FOLLOWUP] Handle unknown worker should also reply WorkerLostResponse
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #1584:
URL: https://github.com/apache/incubator-celeborn/pull/1584#discussion_r1230474245
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala:
##########
@@ -464,13 +464,14 @@ private[celeborn] class Master(
if (worker == null) {
logWarning(s"Unknown worker $host:$rpcPort:$pushPort:$fetchPort:$replicatePort" +
s" for WorkerLost handler!")
- return
- }
-
- statusSystem.handleWorkerLost(host, rpcPort, pushPort, fetchPort, replicatePort, requestId)
-
- if (context != null) {
- context.reply(WorkerLostResponse(true))
+ if (context != null) {
+ context.reply(WorkerLostResponse(false))
Review Comment:
Change to true.
--
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 #1584: [CELEBORN-668][FOLLOWUP] Handle unknown worker should also reply WorkerLostResponse
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #1584:
URL: https://github.com/apache/incubator-celeborn/pull/1584#issuecomment-1589351523
@waitinfuture any concerns? As we are actively changing code in this area, better to make this one get in first
--
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 closed pull request #1584: [CELEBORN-668][FOLLOWUP] Handle unknown worker should also reply WorkerLostResponse
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture closed pull request #1584: [CELEBORN-668][FOLLOWUP] Handle unknown worker should also reply WorkerLostResponse
URL: https://github.com/apache/incubator-celeborn/pull/1584
--
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] AngersZhuuuu commented on pull request #1584: [CELEBORN-668][FOLLOWUP] Handle unknown worker should also reply WorkerLostResponse
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on PR #1584:
URL: https://github.com/apache/incubator-celeborn/pull/1584#issuecomment-1590941687
ping @waitinfuture WDYT?
--
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 a diff in pull request #1584: [CELEBORN-668][FOLLOWUP] Handle unknown worker should also reply WorkerLostResponse
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #1584:
URL: https://github.com/apache/incubator-celeborn/pull/1584#discussion_r1230506832
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala:
##########
@@ -464,13 +464,14 @@ private[celeborn] class Master(
if (worker == null) {
logWarning(s"Unknown worker $host:$rpcPort:$pushPort:$fetchPort:$replicatePort" +
s" for WorkerLost handler!")
- return
- }
-
- statusSystem.handleWorkerLost(host, rpcPort, pushPort, fetchPort, replicatePort, requestId)
-
- if (context != null) {
- context.reply(WorkerLostResponse(true))
+ if (context != null) {
Review Comment:
How about following?
```java
if (worker == null) {
logWarning(s"Unknown worker $host:$rpcPort:$pushPort:$fetchPort:$replicatePort" +
s" for WorkerLost handler!")
} else {
statusSystem.handleWorkerLost(host, rpcPort, pushPort, fetchPort, replicatePort, requestId)
}
if (context != null) {
context.reply(WorkerLostResponse(true))
}
```
--
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 #1584: [CELEBORN-668][FOLLOWUP] Handle unknown worker should also reply WorkerLostResponse
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #1584:
URL: https://github.com/apache/incubator-celeborn/pull/1584#issuecomment-1588708846
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1584?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#1584](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1584?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (3dc864d) into [main](https://app.codecov.io/gh/apache/incubator-celeborn/commit/6177476b18beca2ae20b05b06483c118db74fe5a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (6177476) will **increase** coverage by `0.04%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## main #1584 +/- ##
==========================================
+ Coverage 45.41% 45.44% +0.04%
==========================================
Files 158 158
Lines 9830 9830
Branches 968 968
==========================================
+ Hits 4463 4466 +3
+ Misses 5066 5063 -3
Partials 301 301
```
[see 1 file with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1584/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 a diff in pull request #1584: [CELEBORN-668][FOLLOWUP] Handle unknown worker should also reply WorkerLostResponse
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #1584:
URL: https://github.com/apache/incubator-celeborn/pull/1584#discussion_r1230399522
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala:
##########
@@ -464,13 +464,14 @@ private[celeborn] class Master(
if (worker == null) {
logWarning(s"Unknown worker $host:$rpcPort:$pushPort:$fetchPort:$replicatePort" +
s" for WorkerLost handler!")
- return
- }
-
- statusSystem.handleWorkerLost(host, rpcPort, pushPort, fetchPort, replicatePort, requestId)
-
- if (context != null) {
- context.reply(WorkerLostResponse(true))
+ if (context != null) {
+ context.reply(WorkerLostResponse(false))
Review Comment:
It's ambiguous when reply(WorkerLostResponse with false), how about keep replay true( or false with code-UnKnow).
--
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