[CELEBORN-2222][CIP-14] Support Retrying when createReader failed for CelebornInputStream in CppClient by afterincomparableyum · Pull Request #3583 · apache/celeborn

afterincomparableyum · 2026-01-11T18:28:44Z

This PR implements retry support for createReader failures in the C++ client, matching the behavior of the Java implementation. The implementation includes:

Added configuration properties:
- clientFetchMaxRetriesForEachReplica (default: 3)
- dataIoRetryWait (default: 5s)
- clientPushReplicateEnabled (default: false)
Added peer location support methods to PartitionLocation:
- hasPeer() - Check if location has a peer replica
- getPeer() - Get the peer location
- hostAndFetchPort() - Get host:port string for logging
Implemented retry logic in createReaderWithRetry():
- Retries up to fetchChunkMaxRetry_ times (doubled if replication enabled)[which is why I added this parameter in this PR]
- Switches to peer location on failure when available
- Sleeps between retries when both replicas tried or no peer exists
- Resets retry counter when moving to new location or on success
Added unit tests for new functionality

How was this patch tested?

Unit tests and compiling

afterincomparableyum · 2026-01-11T18:29:48Z

@HolyLow @SteNicholas @FMX @RexXiong Could you please help review this PR? Appreciate your help in improving this as needed!

cpp/celeborn/conf/CelebornConf.h

cpp/celeborn/client/reader/CelebornInputStream.cpp

codecov · 2026-01-20T03:18:39Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.04%. Comparing base (2dd1b7a) to head (5d32d94).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3583      +/-   ##
==========================================
- Coverage   67.13%   67.04%   -0.09%     
==========================================
  Files         357      357              
  Lines       21860    21924      +64     
  Branches     1943     1949       +6     
==========================================
+ Hits        14674    14696      +22     
- Misses       6166     6213      +47     
+ Partials     1020     1015       -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

afterincomparableyum · 2026-01-25T23:30:26Z

Thank you for your comments @SteNicholas , I will take a look over the next couple of days. I suspect some refactoring may need to be done to this PR, I will notify you once done.

Copilot

Pull request overview

This PR implements retry support for createReader failures in the C++ client to match the Java implementation's behavior. It adds retry configuration, peer location helper methods, and implements the retry logic with peer failover.

Changes:

Added three configuration properties for retry behavior: clientFetchMaxRetriesForEachReplica, dataIoRetryWait, and clientPushReplicateEnabled
Added helper methods to PartitionLocation for peer access and formatting: hasPeer(), getPeer(), and hostAndFetchPort()
Implemented retry logic in createReaderWithRetry() that switches between primary and peer replicas on failure

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
cpp/celeborn/protocol/tests/PartitionLocationTest.cpp	Added unit tests for new PartitionLocation helper methods
cpp/celeborn/protocol/PartitionLocation.h	Declared three new helper methods for peer access and port formatting
cpp/celeborn/protocol/PartitionLocation.cpp	Implemented the three new helper methods
cpp/celeborn/conf/tests/CelebornConfTest.cpp	Added tests for new configuration properties and their default values
cpp/celeborn/conf/CelebornConf.h	Declared three new configuration properties and their accessor methods
cpp/celeborn/conf/CelebornConf.cpp	Implemented configuration property definitions and accessor methods
cpp/celeborn/client/reader/CelebornInputStream.h	Added member variables for retry tracking and retry wait timeout
cpp/celeborn/client/reader/CelebornInputStream.cpp	Implemented retry logic with peer failover and sleep between retries

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

cpp/celeborn/client/reader/CelebornInputStream.cpp

afterincomparableyum · 2026-02-08T22:55:38Z

Sorry for delay on this, will push a couple commits within next couple of days

afterincomparableyum · 2026-02-14T18:13:04Z

@SteNicholas I have addressed the comments, can you please take a look again. thank you for your review

afterincomparableyum · 2026-02-15T17:23:58Z

Ping @RexXiong @FMX @HolyLow for review. Thank you for your review!

SteNicholas · 2026-02-16T07:01:32Z

@afterincomparableyum, could you please rebase the main branch to resolve the conflicts?

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

cpp/celeborn/client/reader/CelebornInputStream.cpp

cpp/celeborn/client/ShuffleClient.cpp

cpp/celeborn/client/reader/CelebornInputStream.h

cpp/celeborn/client/reader/CelebornInputStream.cpp

cpp/celeborn/utils/CelebornUtils.cpp

afterincomparableyum · 2026-02-16T20:39:46Z

ping @SteNicholas for review, I have resolved conflicts as well as addressed comments from CoPilot.

SteNicholas · 2026-02-19T08:28:57Z

@afterincomparableyum, could you please rebase the main branch to resolve the conflict?

afterincomparableyum · 2026-02-19T18:43:54Z

@SteNicholas I have rebased

Copilot

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-21T02:54:28Z

cpp/celeborn/client/reader/CelebornInputStream.cpp

+  throw utils::CelebornRuntimeError(
+      lastException,
+      "createPartitionReader failed after " +
+          std::to_string(fetchChunkRetryCnt_) + " retries for location " +
+          location.hostAndFetchPort(),
+      false);


The final wrapped exception message always reports the original location.hostAndFetchPort(), even if the last failure happened after switching to the peer. This can be misleading when debugging. Consider including the last attempted location (e.g., currentLocation->hostAndFetchPort()) or both original+current in the error context.

Suggested change

throw utils::CelebornRuntimeError(

lastException,

"createPartitionReader failed after " +

std::to_string(fetchChunkRetryCnt_) + " retries for location " +

location.hostAndFetchPort(),

false);

std::string errorMessage =

"createPartitionReader failed after " +

std::to_string(fetchChunkRetryCnt_) +

" retries. Original location: " + location.hostAndFetchPort() +

", last attempted location: " + currentLocation->hostAndFetchPort();

throw utils::CelebornRuntimeError(lastException, errorMessage, false);

Copilot · 2026-02-21T02:54:28Z

cpp/celeborn/client/ShuffleClient.h

 #include <functional>
 #include "celeborn/client/compress/Compressor.h"
 #include "celeborn/client/reader/CelebornInputStream.h"
 #include "celeborn/client/writer/PushDataCallback.h"
 #include "celeborn/client/writer/PushState.h"
 #include "celeborn/client/writer/ReviveManager.h"
 #include "celeborn/network/NettyRpcEndpointRef.h"


ShuffleClient.h uses utils::ConcurrentHashMap in several type aliases/member declarations, but it does not include the header that defines it (celeborn/utils/CelebornUtils.h). It currently compiles only because CelebornInputStream.h happens to include CelebornUtils.h; add the direct include here to avoid fragile transitive dependencies.

Copilot · 2026-02-21T02:54:28Z

cpp/celeborn/client/ShuffleClient.h

+  virtual void excludeFailedFetchLocation(
+      const std::string& hostAndFetchPort,
+      const std::exception& e) = 0;
+


The new ShuffleClient::excludeFailedFetchLocation API (and its implementation in ShuffleClientImpl) appears unused in the current codebase (CelebornInputStream maintains exclusion internally instead). If this isn’t intended for external callers, consider removing it to avoid expanding the public interface; otherwise, consider wiring CelebornInputStream to call through ShuffleClient so there is a single source of truth for exclusion behavior.

Suggested change

virtual void excludeFailedFetchLocation(

const std::string& hostAndFetchPort,

const std::exception& e) = 0;

Copilot · 2026-02-21T02:54:28Z

cpp/celeborn/conf/CelebornConf.h

+  static constexpr std::string_view kClientFetchExcludeWorkerOnFailureEnabled{
+      "celeborn.client.fetch.excludeWorkerOnFailure.enabled"};
+
+  static constexpr std::string_view kClientFetchExcludedWorkerExpireTimeout{
+      "celeborn.client.fetch.excludedWorker.expireTimeout"};
+
+  static constexpr std::string_view
+      kClientAdaptiveOptimizeSkewedPartitionReadEnabled{
+          "celeborn.client.adaptive.optimizeSkewedPartitionRead.enabled"};


PR description lists 3 new config properties, but this change also introduces additional fetch-related configs (excludeWorkerOnFailure, excludedWorker expireTimeout, optimizeSkewedPartitionRead). Please update the PR description (or drop these configs if out of scope) so reviewers/users can understand the full config surface change.

Copilot · 2026-02-21T02:54:29Z

cpp/celeborn/client/reader/CelebornInputStream.cpp

 #include "celeborn/client/reader/CelebornInputStream.h"
 #include <lz4.h>
 #include "celeborn/client/compress/Decompressor.h"


CelebornInputStream.cpp uses std::this_thread::sleep_for(retryWait_) later in the file but does not include (and the header no longer provides it). This can fail to compile depending on transitive includes; add an explicit #include in this .cpp.

Copilot · 2026-02-21T02:54:29Z

cpp/celeborn/client/reader/CelebornInputStream.cpp

+      if (isExcluded(*currentLocation)) {
+        CELEBORN_FAIL(
+            "Fetch data from excluded worker! {}",
+            currentLocation->hostAndFetchPort());
+      }


When a location is already in the exclusion list, the code throws and then retries/sleeps (especially in the no-peer branch). Since isExcluded(*currentLocation) will keep returning true until the exclusion expires, these retries are guaranteed to fail and just add delay. Consider failing fast (no sleep/retry) or skipping to another available location/peer when the current location is excluded.

SteNicholas · 2026-02-21T02:59:11Z

@afterincomparableyum, thanks for update. Could you take a look at the review comment of Copilot?

github-actions bot added the module:cpp label Jan 11, 2026

SteNicholas requested changes Jan 20, 2026

View reviewed changes

SteNicholas requested a review from Copilot January 27, 2026 11:48

Copilot started reviewing on behalf of SteNicholas January 27, 2026 11:49 View session

Copilot AI reviewed Jan 27, 2026

View reviewed changes

cpp/celeborn/client/reader/CelebornInputStream.cpp Outdated Show resolved Hide resolved

cpp/celeborn/client/reader/CelebornInputStream.cpp Outdated Show resolved Hide resolved

afterincomparableyum force-pushed the cpp-client/celeborn-2222 branch 4 times, most recently from d46e2f7 to ab9523c Compare February 14, 2026 18:11

afterincomparableyum requested a review from SteNicholas February 14, 2026 18:13

SteNicholas requested a review from Copilot February 16, 2026 07:00

Copilot started reviewing on behalf of SteNicholas February 16, 2026 07:00 View session

Copilot AI reviewed Feb 16, 2026

View reviewed changes

afterincomparableyum force-pushed the cpp-client/celeborn-2222 branch 7 times, most recently from 46de39c to f05d97c Compare February 16, 2026 20:28

afterincomparableyum force-pushed the cpp-client/celeborn-2222 branch 2 times, most recently from 11daa55 to 799136f Compare February 18, 2026 16:58

afterincomparableyum mentioned this pull request Feb 19, 2026

[CELEBORN-2226][CIP-14] Support RetryFetchChunk functionality for Cel… #3605

Draft

afterincomparableyum added 4 commits February 19, 2026 10:25

C++ Client Input Stream Retries

64c2359

address comments

ddc2e1d

fix failure handling in CelebornInputStream#createReaderWithRetry

1087ee9

address comments of Co Pilot AI

35a6870

afterincomparableyum force-pushed the cpp-client/celeborn-2222 branch from 799136f to 35a6870 Compare February 19, 2026 18:26

SteNicholas approved these changes Feb 21, 2026

View reviewed changes

SteNicholas requested a review from Copilot February 21, 2026 02:48

Copilot started reviewing on behalf of SteNicholas February 21, 2026 02:48 View session

Copilot AI reviewed Feb 21, 2026

View reviewed changes

	virtual void excludeFailedFetchLocation(
	const std::string& hostAndFetchPort,
	const std::exception& e) = 0;

Comments

Conversation

afterincomparableyum commented Jan 11, 2026

How was this patch tested?

Uh oh!

afterincomparableyum commented Jan 11, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jan 20, 2026

Codecov Report

Uh oh!

afterincomparableyum commented Jan 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

afterincomparableyum commented Feb 8, 2026

Uh oh!

afterincomparableyum commented Feb 14, 2026

Uh oh!

afterincomparableyum commented Feb 15, 2026

Uh oh!

SteNicholas commented Feb 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

afterincomparableyum commented Feb 16, 2026

Uh oh!

SteNicholas commented Feb 19, 2026

Uh oh!

afterincomparableyum commented Feb 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

Uh oh!

SteNicholas commented Feb 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants