[CELEBORN-2470] Add backoff with jitter to push retries and log hard split once per file

[venkata91]

JIRA: CELEBORN-2470

Summary

Push retries currently run immediately and can synchronize across many tasks, increasing connection and DNS load when a worker is unhealthy. Hard splits can also emit the same INFO log repeatedly while pushes continue targeting the old file.

This PR:

• adds exponential backoff with jitter to push retries
• logs the hard-split cause once per PartitionDataWriter, for both disk and memory files
• moves the repeated merged-push hard-split message to DEBUG

Configuration

Config	Default
celeborn.client.push.retry.backoff.initial	500ms
celeborn.client.push.retry.backoff.max	10s
celeborn.client.push.retry.backoff.jitter	0.5

Setting celeborn.client.push.retry.backoff.initial to 0 disables backoff. celeborn.client.push.retry.backoff.max caps the final delay, including jitter, so with the defaults a retry at the cap waits between 5s and 10s.

Tests

• added coverage for exponential growth, maximum delay, jitter bounds (including at the cap), disabled backoff, and large retry counts
• added callback-level coverage for scheduled single and merged HARD_SPLIT retries and revive completion during backoff
• added concurrent coverage verifying the hard-split log latch triggers once per writer
• added coverage that repeated memory-shuffle HARD_SPLIT checks log only once

[gauravkm]

Looks good from the code review; no P0/P1 issues found. I checked single and merged push retries, revive deadlines and retry budgets, and the atomic per-writer log guard, which leaves split decisions and counters unchanged. Existing retry/split tests do not cover the new backoff timing or log latch yet, and CI currently shows only the two passing label checks.

[venkata91]

Looks good from the code review; no P0/P1 issues found. I checked single and merged push retries, revive deadlines and retry budgets, and the atomic per-writer log guard, which leaves split decisions and counters unchanged. Existing retry/split tests do not cover the new backoff timing or log latch yet, and CI currently shows only the two passing label checks.

Addressed the review comment by adding necessary tests.

[gauravkm]

Thanks for adding the tests. Looks good from the code review; no P0/P1 issues found. I checked the shared backoff helper across single and merged retries, including revive deadlines and retry budgets. The new tests cover exponential growth, the cap, jitter, disabled backoff, large retry counts, and concurrent access to the per-writer log latch. They do not exercise scheduled retry/deadline integration. CI currently shows only the two passing label checks, with no build or test results yet.

[venkata91]

They do not exercise scheduled retry/deadline integration.
@gauravkm can you elaborate what do you mean by that?

[gauravkm]

I meant that testPushRetryBackoff calls pushRetryBackoffMs() directly: it checks the calculated delay, but does not exercise the push callback and scheduled retry together. For example, a focused test could trigger a failed/HARD_SPLIT push with jitter disabled, check the scheduled delay, then control revive completion and verify that successful completion clears the tracked batch or exhausted retries surface the expected failure.

Single pushes calculate the revive deadline before scheduling; merged pushes calculate it inside the scheduled task, so revive completion during backoff versus remaining pending at the deadline are useful cases to cover. This was a coverage suggestion, not a newly identified correctness issue. The helper and latch tests you added are useful.

[gauravkm]

Looks good from the code review; no P0/P1 issues found. I checked that the shared scheduling helper preserves all five retry paths. The new tests address the callback-wiring part of my suggestion: they check the requested 100 ms delay, complete revive before invoking the captured retry, and verify both single and merged resends, plus single-push cleanup. Deadline expiry and retry exhaustion remain outside this coverage. CI currently shows passing label checks only; seven workflows await maintainer approval, so there are no build/test results yet.

[zaynt4606]

Thanks for adding the retry tests. I noticed two remaining edge cases:

1. pushRetryBackoffMs() caps the base delay before applying jitter. With backoff.max=10s and the default jitter of 0.5, the scheduled delay can reach 15s. Could we cap the final delay and test jitter at the cap, or clarify that backoff.max only bounds the base delay?
2. checkDiskFullAndSplit() still logs memory-shuffle HARD_SPLIT at INFO on every call. The new shouldLogHardSplit() guard only covers the disk-file branch. Could we apply it to the memory branch as well and test repeated calls?

[zaynt4606]

Thanks for addressing both review points. One small documentation follow-up: the description of celeborn.client.push.retry.backoff.jitter still says that a value of 0.5 spreads retries over [0.5, 1.5] times the computed delay. With the new final cap, once the base delay reaches backoff.max (10s by default), the actual range is 5–10s, rather than 5–15s. Could you update the config description in CelebornConf.scala and regenerate docs/configuration/client.md to reflect the capped range? The implementation and new cap test look good.

[venkata91]

@zaynt4606 thanks, good catch. Updated in 79fa9fd: the jitter description now says the range is capped at backoff.max (e.g. [5s, 10s] once the computed delay reaches the default 10s), backoff.max now says it bounds the delay including jitter, and docs/configuration/client.md is regenerated. I also updated the PR description to mention the cap and the tests added in the last round.

[zaynt4606]

Thanks @venkata91 for the contribution!
Merged to main(v1.0.0).

[zaynt4606]

LGTM