[CELEBORN-1313] Custom Network Location Aware Replication #2367
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
cc @waitinfuture @otterc @mridulm can you help review? Thanks. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2367 +/- ##
==========================================
- Coverage 48.85% 48.83% -0.01%
==========================================
Files 208 209 +1
Lines 12984 13089 +105
Branches 1115 1133 +18
==========================================
+ Hits 6342 6391 +49
- Misses 6232 6278 +46
- Partials 410 420 +10 ☔ View full report in Codecov by Sentry. |
mridulm
reviewed
Mar 8, 2024
otterc
reviewed
Mar 9, 2024
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Worker.scala
Show resolved
Hide resolved
otterc
reviewed
Mar 9, 2024
...src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java
Outdated
Show resolved
Hide resolved
|
cc @SteNicholas |
|
cc @zwangsheng |
SteNicholas
reviewed
Mar 12, 2024
common/src/test/scala/org/apache/celeborn/common/network/CelebornRackResolverSuite.scala
Show resolved
Hide resolved
...src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java
Show resolved
Hide resolved
...src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java
Outdated
Show resolved
Hide resolved
FMX
approved these changes
Mar 12, 2024
waitinfuture
approved these changes
Mar 12, 2024
...src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java
Show resolved
Hide resolved
zwangsheng
approved these changes
Mar 12, 2024
...src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java
Show resolved
Hide resolved
AngersZhuuuu
approved these changes
Mar 12, 2024
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Worker.scala
Show resolved
Hide resolved
|
addressed the review comments cc @waitinfuture @AngersZhuuuu @FMX @zwangsheng |
|
Thanks, merging to main(v0.5.0) |
FMX
pushed a commit
that referenced
this pull request
Aug 14, 2024
### What changes were proposed in this pull request? Fixing a bug where the `networkLocation` is not persisted in Ratis, and the master defaults to `DEFAULT_RACK` when it loads the snapshot. This was missed in #2367 unfortunately, and it came up during our stress testing internally. ### Why are the changes needed? Needed for custom network aware replication, so that networkLocation state is kept in snapshot file. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Updated unit test to ensure serde is correct. Closes #2669 from akpatnam25/CELEBORN-1549. Authored-by: Aravind Patnam <apatnam@linkedin.com> Signed-off-by: mingji <fengmingxiao.fmx@alibaba-inc.com>
gotikkoxq
added a commit
to gotikkoxq/celeborn
that referenced
this pull request
Aug 26, 2024
### What changes were proposed in this pull request? Fixing a bug where the `networkLocation` is not persisted in Ratis, and the master defaults to `DEFAULT_RACK` when it loads the snapshot. This was missed in apache/celeborn#2367 unfortunately, and it came up during our stress testing internally. ### Why are the changes needed? Needed for custom network aware replication, so that networkLocation state is kept in snapshot file. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Updated unit test to ensure serde is correct. Closes #2669 from akpatnam25/CELEBORN-1549. Authored-by: Aravind Patnam <apatnam@linkedin.com> Signed-off-by: mingji <fengmingxiao.fmx@alibaba-inc.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Enable custom network location aware replication, based on a custom impl of
DNSToSwitchMapping.Why are the changes needed?
Resolution of network location of multiple workers at master can be expensive at times. This way, each worker resolves its own network location and sends to master via the RegisterWorker transport message. If worker cannot resolve, fallback to attempting to resolve at master (during update meta or reload of snapshot). Proposal: Celeborn Custom Network Location Aware Replication
Does this PR introduce any user-facing change?
No
How was this patch tested?
Updated the unit tests.