[RFC][CELEBORN-894] Add support for end to end Integrity Checks #3062

gauravkm · 2025-01-10T19:59:23Z

What changes were proposed in this pull request?

CELEBORN-894
End to End integrity checks reference implementation. Not looking to merge this yet. Looking for high level feedback on the approach

Why are the changes needed?

https://docs.google.com/document/d/1YqK0kua-5rMufJw57kEIrHHGbLnAF9iXM5GdDweMzzg/edit?tab=t.0

Does this PR introduce any user-facing change?

How was this patch tested?

FMX · 2025-01-12T04:38:28Z

Glad to see your PR. If it's ready you can remove the draft label.

gauravkm · 2025-02-03T17:24:31Z

Hi @FMX
Could you please take a look? I removed the draft tag.

FMX · 2025-02-05T06:16:54Z

the

Code review will be done within this week.

FMX

@gauravkm Looks like this PR is incomplete. Are there more PRs for this Jira Ticket?

FMX · 2025-02-06T08:36:35Z

...nt-spark/spark-3/src/main/java/org/apache/spark/shuffle/celeborn/HashBasedShuffleWriter.java

@@ -377,7 +377,8 @@ private void close() throws IOException, InterruptedException {
    updateRecordsWrittenMetrics();

    long waitStartTime = System.nanoTime();
-    shuffleClient.mapperEnd(shuffleId, mapId, encodedAttemptId, numMappers);
+    int bytesWritten = shuffleClient.mapperEnd(shuffleId, mapId, encodedAttemptId, numMappers, numMappers);
+    writeMetrics.incBytesWritten(bytesWritten);


Should not add the bytes written to spark metrics. Because the write metric has the correct value of written bytes.

FMX · 2025-02-06T08:55:36Z

common/src/main/java/org/apache/celeborn/common/write/PushState.java

@@ -32,6 +33,7 @@ public class PushState {
  private final int pushBufferMaxSize;
  public AtomicReference<IOException> exception = new AtomicReference<>();
  private final InFlightRequestTracker inFlightRequestTracker;
+  private final ConcurrentHashMap<Integer, CommitMetadata> commitMetadataMap = new ConcurrentHashMap<>();


This map is always empty. Seems that you forget to update this maps.

FMX · 2025-02-06T09:01:14Z

client/src/main/java/org/apache/celeborn/client/ShuffleClientImpl.java

+    int bytes = 0;
+
+    for (int partitionId = 0; partitionId < numPartitions; partitionId++) {
+      CommitMetadata metadata = metadataMap.getOrDefault(partitionId, new CommitMetadata());


Here will always get empty commit meta data.

gaoyajun02 · 2025-02-26T04:50:14Z

gentle ping @gauravkm
Thank you for your PR. This PR is very valuable to us, but it seems incomplete. Are you still working on it?

gauravkm · 2025-02-26T06:13:35Z

@gaoyajun02 Yes. I am still working on this. We (at Stripe) realized that the implementation needs to be a lot more comprehensive and thorough for the checks to be meaningful and provide confidence. We are internally testing out the new implementation which I will then open for review in OSS as well.
We also found that the current approach introduces a lot of overhead for apps that provision a high partition count but only write to a few of them. So we have altered the design to be able to accommodate such apps.

github-actions · 2025-03-18T08:35:11Z

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.

github-actions · 2025-03-28T08:35:19Z

This issue was closed because it has been staled for 10 days with no activity.

gaoyajun02 · 2025-03-28T12:25:57Z

gentle ping @gauravkm

Since this PR has been closed without updates and this feature is important to us, due to the pressure from our production applications, we hope to accelerate the progress of consistency validation work. We plan to submit a new PR with our implementation. I wanted to check with you one last time before we proceed.

We have an improved solution and implementation ready to submit to the community. However, as it's an enhancement based on your initial approach, we wanted to align with you first.

Please let us know if you have any concerns or if you'd prefer to continue with your implementation.

Thanks for your understanding.

gauravkm · 2025-03-28T15:43:49Z

@gaoyajun02 Apologies that the PR got marked stale and was closed. We do plan to contribute our implementation and are actively testing it out. I am going to ahead and put up a new PR with the updated implementation next week

More detailed answer:
As i mentioned we have been testing out a new optimized implementation internally and it has now been subject to 1000's of Spark apps and multiple petabytes of shuffle running everyday. It also has optimizations around handling for empty partitions to avoid scenarios where commit metadata size overhead was higher than actual shuffle.

We would prefer to continue contributing our implementation. I will put up the new PR next week. Please be rest assured that we are very interested in contributing back our implementation to the community

With respect to the improvements/optimizations you mentioned, would be great if you could contribute them on top of our implementation. Let me try to reach out to you on slack/email to better understand these. Please feel free to comment about the proposed improvements on the design doc as well

Thanks for checking with me and your patience here.

gauravkm · 2025-03-28T16:36:03Z

@gaoyajun02 Could you please drop me an email to gauravkm (at gmail.com) and I am happy to coordinate a 1:1 discussion if you are interested. I am unable to find you on the Celelborn slack or find an email from your github profile.

gaoyajun02 · 2025-03-30T10:15:59Z

@gaoyajun02 Could you please drop me an email to gauravkm (at gmail.com) and I am happy to coordinate a 1:1 discussion if you are interested. I am unable to find you on the Celelborn slack or find an email from your github profile.

Great, I'm really looking forward to seeing the new implementation. If you have plans for next week, I can provide feedback and discussion on your CIP document or new PR in combination with our implementation. @gauravkm

gauravkm · 2025-04-02T05:52:13Z

Thanks!
Working on it this week. Rebased to begin with but having some trouble pushing. Will figure this out tomorrow.

Maybe I should update the design doc as a first step so that you can take a look at the updated design

gauravkm · 2025-04-02T22:45:50Z

Updated the design here - https://docs.google.com/document/d/1YqK0kua-5rMufJw57kEIrHHGbLnAF9iXM5GdDweMzzg/edit?tab=t.0#heading=h.spmltvz1rcvt

gauravkm · 2025-04-04T01:03:48Z

@gaoyajun02 Not sure if you have had a chance to look at the design as yet. Please do take a look and feel free to leave comments.

The PR is still a work in progress. I could not find enough time to get into a reviewable state today and I am OOO tomorrow

I will make progress on putting up the PR next week. Thanks for your continued patience!

gaoyajun02 · 2025-04-07T02:15:55Z

I've reviewed the design document, but I'm still not entirely clear about some details. I'd like to see your PR earlier - could you have it ready today or tomorrow? @gauravkm

gauravkm · 2025-04-07T16:25:11Z

Thanks for reviewing @gaoyajun02!

I am in PST timezone. I won't be able to get it to a polished state today, but I will get it into the state by tomorrow where you can get the details you are looking for in code. I am having to apply a lot of changes in the patch by hand. So its taking time.

gauravkm · 2025-04-07T22:44:37Z

@gaoyajun02 Here you go - https://github.com/apache/celeborn/pull/3204/files

I accidentally opened up this PR in the main repo. For now i have closed it.
Still requires a ton of cleanup and some test related fixes.
But for the purposes of having better understanding the proposed solution this should be good enough
I am going to continue to iterate on the changes in the PR to get them into a more polished state and then put it up for official review

gauravkm · 2025-04-07T22:46:27Z

I will also incorporate feedback from the current PR into the new one as well. (There was some feedback around counters)

[CELEBORN-894] Add support for end to end Integrity Checks

05f204d

gauravkm changed the title ~~[CELEBORN-894] Add support for end to end Integrity Checks~~ [Draft][RFC][CELEBORN-894] Add support for end to end Integrity Checks Jan 10, 2025

gauravkm changed the title ~~[Draft][RFC][CELEBORN-894] Add support for end to end Integrity Checks~~ [RFC][CELEBORN-894] Add support for end to end Integrity Checks Jan 14, 2025

FMX reviewed Feb 6, 2025

View reviewed changes

github-actions bot added the stale label Mar 18, 2025

github-actions bot closed this Mar 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC][CELEBORN-894] Add support for end to end Integrity Checks #3062

[RFC][CELEBORN-894] Add support for end to end Integrity Checks #3062

gauravkm commented Jan 10, 2025 •

edited

Loading

FMX commented Jan 12, 2025

gauravkm commented Feb 3, 2025

FMX commented Feb 5, 2025

FMX left a comment

FMX Feb 6, 2025

FMX Feb 6, 2025

FMX Feb 6, 2025

gaoyajun02 commented Feb 26, 2025

gauravkm commented Feb 26, 2025 •

edited

Loading

github-actions bot commented Mar 18, 2025

github-actions bot commented Mar 28, 2025

gaoyajun02 commented Mar 28, 2025 •

edited

Loading

gauravkm commented Mar 28, 2025 •

edited

Loading

gauravkm commented Mar 28, 2025 •

edited

Loading

gaoyajun02 commented Mar 30, 2025

gauravkm commented Apr 2, 2025

gauravkm commented Apr 2, 2025

gauravkm commented Apr 4, 2025

gaoyajun02 commented Apr 7, 2025 •

edited

Loading

gauravkm commented Apr 7, 2025

gauravkm commented Apr 7, 2025 •

edited

Loading

gauravkm commented Apr 7, 2025

[RFC][CELEBORN-894] Add support for end to end Integrity Checks #3062

[RFC][CELEBORN-894] Add support for end to end Integrity Checks #3062

Conversation

gauravkm commented Jan 10, 2025 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

FMX commented Jan 12, 2025

gauravkm commented Feb 3, 2025

FMX commented Feb 5, 2025

FMX left a comment

Choose a reason for hiding this comment

FMX Feb 6, 2025

Choose a reason for hiding this comment

FMX Feb 6, 2025

Choose a reason for hiding this comment

FMX Feb 6, 2025

Choose a reason for hiding this comment

gaoyajun02 commented Feb 26, 2025

gauravkm commented Feb 26, 2025 • edited Loading

github-actions bot commented Mar 18, 2025

github-actions bot commented Mar 28, 2025

gaoyajun02 commented Mar 28, 2025 • edited Loading

gauravkm commented Mar 28, 2025 • edited Loading

gauravkm commented Mar 28, 2025 • edited Loading

gaoyajun02 commented Mar 30, 2025

gauravkm commented Apr 2, 2025

gauravkm commented Apr 2, 2025

gauravkm commented Apr 4, 2025

gaoyajun02 commented Apr 7, 2025 • edited Loading

gauravkm commented Apr 7, 2025

gauravkm commented Apr 7, 2025 • edited Loading

gauravkm commented Apr 7, 2025

gauravkm commented Jan 10, 2025 •

edited

Loading

gauravkm commented Feb 26, 2025 •

edited

Loading

gaoyajun02 commented Mar 28, 2025 •

edited

Loading

gauravkm commented Mar 28, 2025 •

edited

Loading

gauravkm commented Mar 28, 2025 •

edited

Loading

gaoyajun02 commented Apr 7, 2025 •

edited

Loading

gauravkm commented Apr 7, 2025 •

edited

Loading