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/04/23 06:49:49 UTC
[GitHub] [incubator-celeborn] AngersZhuuuu opened a new pull request, #1450: [CELEBORN-544] Remove unnecessary PbUnregisterShuffleResponse between LifecycleManager and Master
AngersZhuuuu opened a new pull request, #1450:
URL: https://github.com/apache/incubator-celeborn/pull/1450
### What changes were proposed in this pull request?
UnregisterShuffleResponse is not used. remove it.
### 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] pan3793 commented on pull request #1450: [CELEBORN-544] Remove unnecessary PbUnregisterShuffleResponse between LifecycleManager and Master
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #1450:
URL: https://github.com/apache/incubator-celeborn/pull/1450#issuecomment-1518977763
How improvement can gain after dropping the response?
--
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 closed pull request #1450: [CELEBORN-544] Remove unnecessary PbUnregisterShuffleResponse between LifecycleManager and Master
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu closed pull request #1450: [CELEBORN-544] Remove unnecessary PbUnregisterShuffleResponse between LifecycleManager and Master
URL: https://github.com/apache/incubator-celeborn/pull/1450
--
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 #1450: [CELEBORN-544] Remove unnecessary PbUnregisterShuffleResponse between LifecycleManager and Master
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on PR #1450:
URL: https://github.com/apache/incubator-celeborn/pull/1450#issuecomment-1518988767
Ok
--
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 #1450: [CELEBORN-544] Remove unnecessary PbUnregisterShuffleResponse between LifecycleManager and Master
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #1450:
URL: https://github.com/apache/incubator-celeborn/pull/1450#issuecomment-1518969090
## [Codecov](https://codecov.io/gh/apache/incubator-celeborn/pull/1450?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1450](https://codecov.io/gh/apache/incubator-celeborn/pull/1450?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d474d5f) into [main](https://codecov.io/gh/apache/incubator-celeborn/commit/e3ae2f0e17939ed7f765edea248b55d641603600?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e3ae2f0) will **increase** coverage by `0.12%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## main #1450 +/- ##
==========================================
+ Coverage 44.87% 44.98% +0.12%
==========================================
Files 156 155 -1
Lines 9596 9587 -9
Branches 956 953 -3
==========================================
+ Hits 4305 4312 +7
+ Misses 5008 4995 -13
+ Partials 283 280 -3
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-celeborn/pull/1450?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...che/celeborn/common/haclient/RssHARetryClient.java](https://codecov.io/gh/apache/incubator-celeborn/pull/1450?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9jZWxlYm9ybi9jb21tb24vaGFjbGllbnQvUnNzSEFSZXRyeUNsaWVudC5qYXZh) | `69.08% <0.00%> (-6.20%)` | :arrow_down: |
| [...born/common/protocol/message/ControlMessages.scala](https://codecov.io/gh/apache/incubator-celeborn/pull/1450?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL3Byb3RvY29sL21lc3NhZ2UvQ29udHJvbE1lc3NhZ2VzLnNjYWxh) | `1.53% <ø> (+0.02%)` | :arrow_up: |
... and [3 files with indirect coverage changes](https://codecov.io/gh/apache/incubator-celeborn/pull/1450/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
: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=The+Apache+Software+Foundation)
--
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 #1450: [CELEBORN-544] Remove unnecessary PbUnregisterShuffleResponse between LifecycleManager and Master
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on PR #1450:
URL: https://github.com/apache/incubator-celeborn/pull/1450#issuecomment-1518966465
ping @RexXiong @waitinfuture @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 pull request #1450: [CELEBORN-544] Remove unnecessary PbUnregisterShuffleResponse between LifecycleManager and Master
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #1450:
URL: https://github.com/apache/incubator-celeborn/pull/1450#issuecomment-1518968359
Seems it breaks the protocol contract.
--
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 #1450: [CELEBORN-544] Remove unnecessary PbUnregisterShuffleResponse between LifecycleManager and Master
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #1450:
URL: https://github.com/apache/incubator-celeborn/pull/1450#issuecomment-1518969227
Assuming a 0.2 client communicates to 0.3 master, the client can not receive the response but got the next message returned by the server, then the channel is broken.
--
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 #1450: [CELEBORN-544] Remove unnecessary PbUnregisterShuffleResponse between LifecycleManager and Master
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #1450:
URL: https://github.com/apache/incubator-celeborn/pull/1450#issuecomment-1518977570
> So how about only removing client-side code in this version?
I'm afraid it does not work either. The messages are pipelined in the channel, if we really decide to do this:
0.3
- the server keeps no change
- the client changes the `ssHARetryClient.askSync[PbUnregisterShuffleResponse]` to `ssHARetryClient.send`, but keeps the decode code, consumes, and drops the response message silently.
0.4
- remove the response message totally, both on client and server side
--
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 #1450: [CELEBORN-544] Remove unnecessary PbUnregisterShuffleResponse between LifecycleManager and Master
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on PR #1450:
URL: https://github.com/apache/incubator-celeborn/pull/1450#issuecomment-1518971344
> Assuming a 0.2 client communicates to 0.3 master, the client can not receive the response but got the next message returned by the server, then the channel is broken.
So how about only removing client-side code in this version?
--
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 #1450: [CELEBORN-544] Remove unnecessary PbUnregisterShuffleResponse between LifecycleManager and Master
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on PR #1450:
URL: https://github.com/apache/incubator-celeborn/pull/1450#issuecomment-1518978401
Maybe just quick return, don;t need to wait for reply.
--
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 #1450: [CELEBORN-544] Remove unnecessary PbUnregisterShuffleResponse between LifecycleManager and Master
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #1450:
URL: https://github.com/apache/incubator-celeborn/pull/1450#issuecomment-1518981667
Overall, I oppose this change. The benefit looks not much comparing the impacts, unless it is proven to perform much well under certain conditions.
--
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