Re: [PR] [CELEBORN-2343] Fix timer leak in handlePushData [celeborn]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #3709:
URL: https://github.com/apache/celeborn/pull/3709#discussion_r3341640404


##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/PushDataHandler.scala:
##########
@@ -256,7 +256,7 @@ class PushDataHandler(val workerSource: WorkerSource) extends BaseMessageHandler
     val splitStatus = checkDiskFullAndSplit(fileWriter, isPrimary)
     if (splitStatus == StatusCode.HARD_SPLIT) {
       workerSource.incCounter(WorkerSource.WRITE_DATA_HARD_SPLIT_COUNT)
-      callback.onSuccess(ByteBuffer.wrap(Array[Byte](StatusCode.HARD_SPLIT.getValue)))
+      callbackWithTimer.onSuccess(ByteBuffer.wrap(Array[Byte](StatusCode.HARD_SPLIT.getValue)))
       return

Review Comment:
   The timer-leak fix is behavioral (ensures the `RpcResponseCallbackWithTimer` path is used on HARD_SPLIT / non-SUCCESS early responses), but there’s no accompanying test to prevent regressions. Given there is already a `PushDataHandlerSuite`, consider adding a unit test that asserts `WorkerSource.stopTimer(...)` is invoked for these early-return branches (e.g., HARD_SPLIT from `checkDiskFullAndSplit`, and `result(0) != SUCCESS`).



-- 
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