Copilot commented on code in PR #3845:
URL: https://github.com/apache/celeborn/pull/3845#discussion_r4004617604
##########
common/src/main/java/org/apache/celeborn/common/protocol/message/StatusCode.java:
##########
@@ -93,7 +93,8 @@ public enum StatusCode {
SEGMENT_START_FAIL_PRIMARY(53),
NO_SPLIT(54),
WORKER_UNRESPONSIVE(55),
- READ_REDUCER_PARTITION_END_FAILED(56);
+ READ_REDUCER_PARTITION_END_FAILED(56),
+ SHUFFLE_EXPIRED(57);
Review Comment:
This adds a wire status that the C++ client cannot deserialize.
`cpp/celeborn/protocol/StatusCode.h` still ends at `TAIL == 55`, and
`cpp/celeborn/protocol/PartitionLocation.cpp:175-178` rejects any response
status above that value, so a C++ `GetReducerFileGroup` receiving 57 will abort
before it can handle the expired shuffle. Update the C++ enum/conversion bound
and `ShuffleClientImpl` handling together with this protocol change.
##########
client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala:
##########
@@ -1280,6 +1290,7 @@ class LifecycleManager(val appUniqueId: String, val conf:
CelebornConf) extends
}
}
celebornShuffleIdToAppShuffleIdMap.remove(shuffleId)
+ expiredShuffleIds.add(shuffleId)
Review Comment:
`unregisterShuffle` is also called for lazy shuffles that never reached
`registeredShuffle` (for example the `hasMapping = false` app-unregister path).
Adding every such id to this set makes a later `GetReducerFileGroup` return
`SHUFFLE_EXPIRED`, losing the existing `SHUFFLE_UNREGISTERED`/empty-shuffle
behavior. Only record ids that were actually registered (or otherwise
distinguish an unregistered empty shuffle).
This issue also appears on line 1310 of the same file.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
