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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
