You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "afterincomparableyum (via GitHub)" <gi...@apache.org> on 2026/06/12 04:02:26 UTC
[PR] [CIP-14][CELEBORN-2220] Support Slow Start Push in C++ Client [celeborn]
afterincomparableyum opened a new pull request, #3730:
URL: https://github.com/apache/celeborn/pull/3730
<!--
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?
Implement SlowStartPushStrategy in the C++ client, mirroring the Java client's TCP-like congestion control for push speed:
- Track a per worker CongestControlContext that grows the in-flight request limit on success (slow start, then congestion avoidance once the limit reaches the threshold) and halves it on congestion, falling back to sleeping when congestion persists at a limit of 1.
- Select the strategy through celeborn.client.push.limitStrategy=SLOWSTART
- Add configs celeborn.client.push.slowStart.initialSleepTime (default 500ms) and celeborn.client.push.slowStart.maxSleepTime (default 2s) to control sleep intervals while ramping up.
- Add PushStrategyTest covering strategy creation, sleep time calculation, congestion control, and per host isolation, and add to CelebornConfTest with the new configs.
### Why are the changes needed?
This is needed to bridge the gap between features that the java client has vs what the c++ client has.
### Does this PR resolve a correctness bug?
<!-- Check if yes. The `correctness` label will be added/removed automatically. -->
- [ ] Yes
### Does this PR introduce _any_ user-facing change?
<!-- Check if yes. -->
- [ ] Yes
### How was this patch tested?
CI/CD
--
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
Re: [PR] [CIP-14][CELEBORN-2220] Support Slow Start Push in C++ Client [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on PR #3730:
URL: https://github.com/apache/celeborn/pull/3730#issuecomment-4705977326
Merged to main(v0.7.0).
--
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
Re: [PR] [CIP-14][CELEBORN-2220] Support Slow Start Push in C++ Client [celeborn]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3730:
URL: https://github.com/apache/celeborn/pull/3730#issuecomment-5422850877
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
Re: [PR] [CIP-14][CELEBORN-2220] Support Slow Start Push in C++ Client [celeborn]
Posted by "s0nskar (via GitHub)" <gi...@apache.org>.
s0nskar commented on PR #3730:
URL: https://github.com/apache/celeborn/pull/3730#issuecomment-4728545164
## Summary
Replaces the hardcoded `metrics_` prefix in metric names with a configurable
`celeborn.metrics.prefix` (default `"metrics"`). Small, focused change touching
`CelebornConf`, `AbstractSource.normalizeKey`, docs, and a test.
## Verdict
Solid, mergeable with one substantive concern.
## What's good
- **Backward compatible.** Default is `"metrics"`, so `normalizeKey` produces the
exact same output (`metrics_abc_`) as before. Every other `"metrics_"` reference
in the codebase lives in test files that rely on the default — none break.
- **Version is correct.** `.version("0.7.0")` matches `project.version = 0.7.0-SNAPSHOT`
in `pom.xml`.
- **Single point of change.** `normalizeKey` is the only place metric names are built
(5 callers, all in `AbstractSource`), so the change is complete — no missed call sites.
- **Test + docs included**, and the existing default-prefix test
(`CelebornSourceSuite.scala:38`) implicitly guards backward compatibility.
## Inline comments
### 1. The prefix is not sanitized (substantive)
**File:** `common/src/main/scala/org/apache/celeborn/common/metrics/source/AbstractSource.scala`
**Line:** 704
```scala
s"${metricsPrefix}_${key.replaceAll("[^a-zA-Z0-9]", "_")}_"
```
The metric `key` is sanitized to `[a-zA-Z0-9_]`, but the user-supplied `metricsPrefix`
is interpolated raw. The hardcoded `"metrics"` was always alphanumeric; an arbitrary
user value may not be. Prometheus metric names must match `[a-zA-Z_:][a-zA-Z0-9_:]*`,
so a natural choice like `celeborn.metrics.prefix = my-app` produces `my-app_abc_Count`
— an **invalid Prometheus name that silently breaks scraping**. This is a regression in
robustness from the previously-guaranteed-valid hardcoded prefix.
Recommend one of:
1. Apply the same sanitization to the prefix:
```scala
s"${metricsPrefix.replaceAll("[^a-zA-Z0-9]", "_")}_${key.replaceAll("[^a-zA-Z0-9]", "_")}_"
```
2. Or validate at config time with `.checkValue(...)` on `METRICS_PREFIX` to reject
invalid prefixes early with a clear error.
Option 1 is the most user-forgiving and matches the existing key-handling intent.
### 2. Empty prefix yields a leading underscore (minor)
**File:** `common/src/main/scala/org/apache/celeborn/common/metrics/source/AbstractSource.scala`
**Line:** 704
An empty prefix (`""`) yields a leading-underscore name `_abc_Count` — valid Prometheus,
but worth noting in the doc, or guard against it if undesired.
### 3. Document the prefix constraint (minor)
**File:** `common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala`
**Lines:** 5905–5912
```scala
val METRICS_PREFIX: ConfigEntry[String] =
buildConf("celeborn.metrics.prefix")
.categories("metrics")
.doc("Prefix metrics with this value.")
.version("0.7.0")
.stringConf
.createWithDefault("metrics")
```
The doc string `"Prefix metrics with this value."` could mention the alphanumeric
constraint so users don't trip over it (also regenerate `docs/configuration/metrics.md`
if the doc string changes).
### 4. Add a test for a "dirty" prefix (minor)
**File:** `common/src/test/scala/org/apache/celeborn/common/metrics/source/CelebornSourceSuite.scala`
**Lines:** 41–58
The new test only covers a clean prefix (`"celeborn"`). If you adopt sanitization
(comment #1), add a case asserting a dirty prefix (e.g. `"my-app"`) gets normalized —
that's exactly the behavior most likely to regress later.
– _Reviewed by Claude Code_
--
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
Re: [PR] [CIP-14][CELEBORN-2220] Support Slow Start Push in C++ Client [celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #3730:
URL: https://github.com/apache/celeborn/pull/3730#issuecomment-4687630429
## [Codecov](https://app.codecov.io/gh/apache/celeborn/pull/3730?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 57.55%. Comparing base ([`b4cb5a0`](https://app.codecov.io/gh/apache/celeborn/commit/b4cb5a0b1ac097d33baf8dded1b5be2afd0578a4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`dd40144`](https://app.codecov.io/gh/apache/celeborn/commit/dd401443962d0f3b33338a6b94a704ef0268edb8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 64 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #3730 +/- ##
============================================
- Coverage 66.91% 57.55% -9.36%
- Complexity 0 214 +214
============================================
Files 358 395 +37
Lines 21986 27811 +5825
Branches 1946 2710 +764
============================================
+ Hits 14710 16003 +1293
- Misses 6262 10657 +4395
- Partials 1014 1151 +137
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/celeborn/pull/3730?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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
Re: [PR] [CIP-14][CELEBORN-2220] Support Slow Start Push in C++ Client [celeborn]
Posted by "afterincomparableyum (via GitHub)" <gi...@apache.org>.
afterincomparableyum commented on PR #3730:
URL: https://github.com/apache/celeborn/pull/3730#issuecomment-4829364361
Ping @SteNicholas @RexXiong could one of y'all take a look please.
--
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
Re: [PR] [CIP-14][CELEBORN-2220] Support Slow Start Push in C++ Client [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas closed pull request #3730: [CIP-14][CELEBORN-2220] Support Slow Start Push in C++ Client
URL: https://github.com/apache/celeborn/pull/3730
--
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
Re: [PR] [CIP-14][CELEBORN-2220] Support Slow Start Push in C++ Client [celeborn]
Posted by "afterincomparableyum (via GitHub)" <gi...@apache.org>.
afterincomparableyum commented on PR #3730:
URL: https://github.com/apache/celeborn/pull/3730#issuecomment-5708947996
will start resuming working on this PR soon.
--
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
Re: [PR] [CIP-14][CELEBORN-2220] Support Slow Start Push in C++ Client [celeborn]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3730:
URL: https://github.com/apache/celeborn/pull/3730#issuecomment-6077610078
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