Copilot commented on code in PR #3653:
URL: https://github.com/apache/celeborn/pull/3653#discussion_r3044925821
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/PushDataHandler.scala:
##########
@@ -1456,8 +1456,10 @@ class PushDataHandler(val workerSource: WorkerSource)
extends BaseMessageHandler
|""".stripMargin)
val diskFileInfo = fileWriter.getDiskFileInfo
if (diskFileInfo != null) {
- if (workerPartitionSplitEnabled && ((diskFull &&
diskFileInfo.getFileLength > partitionSplitMinimumSize) ||
- (isPrimary && diskFileInfo.getFileLength >
fileWriter.getSplitThreshold))) {
+ if (workerPartitionSplitEnabled && diskFull &&
(diskFileInfo.getFileLength >= partitionSplitMinimumSize)) {
Review Comment:
This changes the diskFull split threshold from `> partitionSplitMinimumSize`
to `>= partitionSplitMinimumSize`. If the intent of this PR is only to force
HARD_SPLIT (not change the minimum-size boundary), consider keeping the strict
`>` comparison or add a brief comment/commit message rationale for why equality
should now trigger a split.
```suggestion
if (workerPartitionSplitEnabled && diskFull &&
(diskFileInfo.getFileLength > partitionSplitMinimumSize)) {
```
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/PushDataHandler.scala:
##########
@@ -1456,8 +1456,10 @@ class PushDataHandler(val workerSource: WorkerSource)
extends BaseMessageHandler
|""".stripMargin)
val diskFileInfo = fileWriter.getDiskFileInfo
if (diskFileInfo != null) {
- if (workerPartitionSplitEnabled && ((diskFull &&
diskFileInfo.getFileLength > partitionSplitMinimumSize) ||
- (isPrimary && diskFileInfo.getFileLength >
fileWriter.getSplitThreshold))) {
+ if (workerPartitionSplitEnabled && diskFull &&
(diskFileInfo.getFileLength >= partitionSplitMinimumSize)) {
+ return StatusCode.HARD_SPLIT
+ }
Review Comment:
When diskFull triggers a HARD_SPLIT here, the method returns without any
info/warn log (only the earlier logTrace). In production, trace is often
disabled, so disk-full driven splits may be hard to diagnose. Consider adding a
logInfo/logWarning in this branch (include mount point / usable space if
available) similar to the hard-split log below.
--
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]
