You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "zaynt4606 (via GitHub)" <gi...@apache.org> on 2026/04/07 11:52:17 UTC
[PR] [CELEBORN-2287] Split mode should be hard when diskFull [celeborn]
zaynt4606 opened a new pull request, #3653:
URL: https://github.com/apache/celeborn/pull/3653
<!--
Thanks for sending a pull request! Here are some tips for you:
- Make sure the PR title start w/ a JIRA ticket, e.g. '[CELEBORN-XXXX] Your PR title ...'.
- Be sure to keep the PR description updated to reflect all changes.
- Please write your PR title to summarize what this PR proposes.
- If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
### Why are the changes needed?
### Does this PR resolve a correctness bug?
No
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
manul test
--
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
Re: [PR] [CELEBORN-2287] Split mode should be hard when diskFull [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on PR #3653:
URL: https://github.com/apache/celeborn/pull/3653#issuecomment-4203552511
@zaynt4606, please update the description of this pull request.
--
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
Re: [PR] [CELEBORN-2287] Split mode should be HARD_SPLIT when disk is full [celeborn]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3653:
URL: https://github.com/apache/celeborn/pull/3653#discussion_r3057887472
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/PushDataHandler.scala:
##########
@@ -1425,7 +1425,7 @@ class PushDataHandler(val workerSource: WorkerSource) extends BaseMessageHandler
(false, fileWriter)
}
- private def checkDiskFull(fileWriter: PartitionDataWriter): Boolean = {
+ private[worker] def checkDiskFull(fileWriter: PartitionDataWriter): Boolean = {
Review Comment:
Both `checkDiskFull` and `checkDiskFullAndSplit` are widened to `private[worker]`. If the goal is only to unit test the split decision, consider limiting the visibility change to just `checkDiskFullAndSplit` (or extracting the decision logic into a small package-private helper) to reduce the surface area exposed to other production code in the `worker` package.
```suggestion
private def checkDiskFull(fileWriter: PartitionDataWriter): Boolean = {
```
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/PushDataHandler.scala:
##########
@@ -1455,9 +1455,11 @@ class PushDataHandler(val workerSource: WorkerSource) extends BaseMessageHandler
|fileName:${fileWriter.getCurrentFileInfo.getFilePath}
|""".stripMargin)
val diskFileInfo = fileWriter.getDiskFileInfo
- if (diskFileInfo != null) {
- if (workerPartitionSplitEnabled && ((diskFull && diskFileInfo.getFileLength > partitionSplitMinimumSize) ||
- (isPrimary && diskFileInfo.getFileLength > fileWriter.getSplitThreshold))) {
+ if (workerPartitionSplitEnabled && diskFileInfo != null) {
+ if (diskFull && (diskFileInfo.getFileLength > partitionSplitMinimumSize)) {
+ return StatusCode.HARD_SPLIT
+ }
+ if (isPrimary && diskFileInfo.getFileLength > fileWriter.getSplitThreshold) {
if (fileWriter.getSplitMode == PartitionSplitMode.SOFT &&
(fileWriter.getDiskFileInfo.getFileLength < partitionSplitMaximumSize)) {
Review Comment:
Inside the `diskFileInfo != null` branch, the code re-fetches `fileWriter.getDiskFileInfo` at line 1464 even though `diskFileInfo` is already available and non-null. Use the local `diskFileInfo` consistently to avoid redundant calls and to ensure the decision is based on a single, consistent snapshot of file length.
```suggestion
(diskFileInfo.getFileLength < partitionSplitMaximumSize)) {
```
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/PushDataHandler.scala:
##########
@@ -1425,7 +1425,7 @@ class PushDataHandler(val workerSource: WorkerSource) extends BaseMessageHandler
(false, fileWriter)
}
- private def checkDiskFull(fileWriter: PartitionDataWriter): Boolean = {
+ private[worker] def checkDiskFull(fileWriter: PartitionDataWriter): Boolean = {
val flusher = fileWriter.getFlusher;
Review Comment:
Scala style convention generally avoids trailing semicolons. Consider removing the semicolon to match typical Scala code style (and reduce diff noise in future edits).
```suggestion
val flusher = fileWriter.getFlusher
```
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/PushDataHandler.scala:
##########
@@ -1436,7 +1436,7 @@ class PushDataHandler(val workerSource: WorkerSource) extends BaseMessageHandler
}
}
- private def checkDiskFullAndSplit(
+ private[worker] def checkDiskFullAndSplit(
Review Comment:
Both `checkDiskFull` and `checkDiskFullAndSplit` are widened to `private[worker]`. If the goal is only to unit test the split decision, consider limiting the visibility change to just `checkDiskFullAndSplit` (or extracting the decision logic into a small package-private helper) to reduce the surface area exposed to other production code in the `worker` package.
--
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
Re: [PR] [CELEBORN-2287] Split mode should be HARD_SPLIT when disk is full [celeborn]
Posted by "zaynt4606 (via GitHub)" <gi...@apache.org>.
zaynt4606 commented on code in PR #3653:
URL: https://github.com/apache/celeborn/pull/3653#discussion_r3058631000
##########
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:
> In case of disk full, do we need check `diskFileInfo.getFileLength >= partitionSplitMinimumSize` ? I think main priority should be save the remaining buffer disk space and send HARD_SPLIT for every partition.
When partitionSplitMinimumSize is only 1MB, its impact on the remaining buffer disk space is negligible. Not splitting such tiny data chunks is also a performance trade-off, so the original logic remains unchanged.
--
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
Re: [PR] [CELEBORN-2287] Split mode should be HARD_SPLIT when disk is full [celeborn]
Posted by "s0nskar (via GitHub)" <gi...@apache.org>.
s0nskar commented on code in PR #3653:
URL: https://github.com/apache/celeborn/pull/3653#discussion_r3057849461
##########
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:
In case of disk full, do we need check `diskFileInfo.getFileLength >= partitionSplitMinimumSize` ? I think main priority should be save the remaining buffer disk space and send HARD_SPLIT for every partition.
--
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
Re: [PR] [CELEBORN-2287] Split mode should be hard when diskFull [celeborn]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2287] Split mode should be HARD_SPLIT when disk is full [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas closed pull request #3653: [CELEBORN-2287] Split mode should be HARD_SPLIT when disk is full
URL: https://github.com/apache/celeborn/pull/3653
--
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
Re: [PR] [CELEBORN-2287] Split mode should be HARD_SPLIT when disk is full [celeborn]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3653:
URL: https://github.com/apache/celeborn/pull/3653#discussion_r3061556699
##########
worker/src/test/scala/org/apache/celeborn/service/deploy/worker/PushDataHandlerSuite.scala:
##########
@@ -0,0 +1,279 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.service.deploy.worker
+
+import java.util
+
+import org.mockito.MockitoSugar._
+import org.scalatest.funsuite.AnyFunSuite
+
+import org.apache.celeborn.common.CelebornConf
+import org.apache.celeborn.common.meta.{DiskFileInfo, DiskInfo, MemoryFileInfo, WorkerInfo}
+import org.apache.celeborn.common.meta.DiskStatus
+import org.apache.celeborn.common.protocol.PartitionSplitMode
+import org.apache.celeborn.common.protocol.message.StatusCode
+import org.apache.celeborn.service.deploy.worker.storage.{LocalFlusher, PartitionDataWriter}
+
+/**
+ * Unit tests for [[PushDataHandler.checkDiskFullAndSplit]].
+ *
+ * The method under test has the following decision tree:
+ * 1. If needHardSplitForMemoryShuffleStorage() → HARD_SPLIT
+ * 2. If workerPartitionSplitEnabled is false or diskFileInfo is null → NO_SPLIT
+ * 3. If disk is full and fileLength > partitionSplitMinimumSize → HARD_SPLIT
+ * 4. If isPrimary and fileLength > splitThreshold:
+ * a. SOFT mode and fileLength < partitionSplitMaximumSize → SOFT_SPLIT
+ * b. Otherwise (HARD mode, or fileLength >= partitionSplitMaximumSize) → HARD_SPLIT
+ * 5. Otherwise → NO_SPLIT
+ */
+class PushDataHandlerSuite extends AnyFunSuite {
+
+ // ── helpers ──────────────────────────────────────────────────────────────
+
+ private val splitMinimumSize: Long = 1024L * 1024L // 1 MB
+ private val splitMaximumSize: Long = 1024L * 1024L * 10L // 10 MB
+ private val splitThreshold: Long = 1024L * 1024L * 2L // 2 MB
+ private val mountPoint = "/mnt/disk0"
+
+ /**
+ * Build a [[PushDataHandler]] with the given configuration flags, wired up
+ * with a minimal [[WorkerInfo]] that contains one disk entry for [[mountPoint]].
+ */
+ private def buildHandler(
+ partitionSplitEnabled: Boolean,
+ diskStatus: DiskStatus = DiskStatus.HEALTHY,
+ actualUsableSpace: Long = Long.MaxValue): PushDataHandler = {
+ val conf = new CelebornConf()
+ conf.set(CelebornConf.WORKER_PARTITION_SPLIT_ENABLED.key, partitionSplitEnabled.toString)
+ conf.set(CelebornConf.WORKER_PARTITION_SPLIT_MIN_SIZE.key, splitMinimumSize.toString)
+ conf.set(CelebornConf.WORKER_PARTITION_SPLIT_MAX_SIZE.key, splitMaximumSize.toString)
+
+ val diskInfo = new DiskInfo(mountPoint, actualUsableSpace, 0L, 0L, 0L)
+ diskInfo.setStatus(diskStatus)
+
+ val diskInfoMap = new util.HashMap[String, DiskInfo]()
+ diskInfoMap.put(mountPoint, diskInfo)
+
+ val workerInfo = new WorkerInfo("localhost", 9095, 9096, 9097, 9098, -1, diskInfoMap, null)
+
+ val workerSource = mock[WorkerSource]
+
+ val handler = new PushDataHandler(workerSource)
+ // Inject fields that checkDiskFullAndSplit depends on via reflection helpers
+ // exposed through the init path. We use a minimal mock Worker instead.
+ val mockWorker = mock[Worker]
+ when(mockWorker.conf).thenReturn(conf)
+ when(mockWorker.workerInfo).thenReturn(workerInfo)
+ // Provide no-op stubs for all other Worker fields accessed during init
+ when(mockWorker.partitionLocationInfo).thenReturn(mock[
+ org.apache.celeborn.common.meta.WorkerPartitionLocationInfo])
+ when(mockWorker.shufflePartitionType).thenReturn(
+ new java.util.concurrent.ConcurrentHashMap())
+ when(mockWorker.shufflePushDataTimeout).thenReturn(
+ new java.util.concurrent.ConcurrentHashMap())
+ when(mockWorker.shuffleMapperAttempts).thenReturn(
+ new java.util.concurrent.ConcurrentHashMap())
Review Comment:
These ConcurrentHashMap instances are created without type parameters. In Scala this infers ConcurrentHashMap[Nothing, Nothing], which won’t typecheck against Worker’s expected field types (e.g., ConcurrentHashMap[String, PartitionType], ConcurrentHashMap[String, Long], ConcurrentHashMap[String, AtomicIntegerArray]). Please construct them with the proper type parameters or use JavaUtils.newConcurrentHashMap[T,U] to match Worker.scala’s definitions.
```suggestion
new java.util.concurrent.ConcurrentHashMap[
String,
java.util.concurrent.atomic.AtomicIntegerArray]())
```
##########
worker/src/test/scala/org/apache/celeborn/service/deploy/worker/PushDataHandlerSuite.scala:
##########
@@ -0,0 +1,279 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.service.deploy.worker
+
+import java.util
+
+import org.mockito.MockitoSugar._
+import org.scalatest.funsuite.AnyFunSuite
+
+import org.apache.celeborn.common.CelebornConf
+import org.apache.celeborn.common.meta.{DiskFileInfo, DiskInfo, MemoryFileInfo, WorkerInfo}
+import org.apache.celeborn.common.meta.DiskStatus
+import org.apache.celeborn.common.protocol.PartitionSplitMode
+import org.apache.celeborn.common.protocol.message.StatusCode
+import org.apache.celeborn.service.deploy.worker.storage.{LocalFlusher, PartitionDataWriter}
+
+/**
+ * Unit tests for [[PushDataHandler.checkDiskFullAndSplit]].
+ *
+ * The method under test has the following decision tree:
+ * 1. If needHardSplitForMemoryShuffleStorage() → HARD_SPLIT
+ * 2. If workerPartitionSplitEnabled is false or diskFileInfo is null → NO_SPLIT
+ * 3. If disk is full and fileLength > partitionSplitMinimumSize → HARD_SPLIT
+ * 4. If isPrimary and fileLength > splitThreshold:
+ * a. SOFT mode and fileLength < partitionSplitMaximumSize → SOFT_SPLIT
+ * b. Otherwise (HARD mode, or fileLength >= partitionSplitMaximumSize) → HARD_SPLIT
+ * 5. Otherwise → NO_SPLIT
+ */
+class PushDataHandlerSuite extends AnyFunSuite {
+
+ // ── helpers ──────────────────────────────────────────────────────────────
+
+ private val splitMinimumSize: Long = 1024L * 1024L // 1 MB
+ private val splitMaximumSize: Long = 1024L * 1024L * 10L // 10 MB
+ private val splitThreshold: Long = 1024L * 1024L * 2L // 2 MB
+ private val mountPoint = "/mnt/disk0"
+
+ /**
+ * Build a [[PushDataHandler]] with the given configuration flags, wired up
+ * with a minimal [[WorkerInfo]] that contains one disk entry for [[mountPoint]].
+ */
+ private def buildHandler(
+ partitionSplitEnabled: Boolean,
+ diskStatus: DiskStatus = DiskStatus.HEALTHY,
+ actualUsableSpace: Long = Long.MaxValue): PushDataHandler = {
+ val conf = new CelebornConf()
+ conf.set(CelebornConf.WORKER_PARTITION_SPLIT_ENABLED.key, partitionSplitEnabled.toString)
+ conf.set(CelebornConf.WORKER_PARTITION_SPLIT_MIN_SIZE.key, splitMinimumSize.toString)
+ conf.set(CelebornConf.WORKER_PARTITION_SPLIT_MAX_SIZE.key, splitMaximumSize.toString)
+
+ val diskInfo = new DiskInfo(mountPoint, actualUsableSpace, 0L, 0L, 0L)
+ diskInfo.setStatus(diskStatus)
+
+ val diskInfoMap = new util.HashMap[String, DiskInfo]()
+ diskInfoMap.put(mountPoint, diskInfo)
+
+ val workerInfo = new WorkerInfo("localhost", 9095, 9096, 9097, 9098, -1, diskInfoMap, null)
+
+ val workerSource = mock[WorkerSource]
+
+ val handler = new PushDataHandler(workerSource)
+ // Inject fields that checkDiskFullAndSplit depends on via reflection helpers
+ // exposed through the init path. We use a minimal mock Worker instead.
+ val mockWorker = mock[Worker]
+ when(mockWorker.conf).thenReturn(conf)
+ when(mockWorker.workerInfo).thenReturn(workerInfo)
+ // Provide no-op stubs for all other Worker fields accessed during init
+ when(mockWorker.partitionLocationInfo).thenReturn(mock[
+ org.apache.celeborn.common.meta.WorkerPartitionLocationInfo])
+ when(mockWorker.shufflePartitionType).thenReturn(
+ new java.util.concurrent.ConcurrentHashMap())
+ when(mockWorker.shufflePushDataTimeout).thenReturn(
+ new java.util.concurrent.ConcurrentHashMap())
+ when(mockWorker.shuffleMapperAttempts).thenReturn(
+ new java.util.concurrent.ConcurrentHashMap())
+ when(mockWorker.replicateThreadPool).thenReturn(mock[java.util.concurrent.ThreadPoolExecutor])
+ when(mockWorker.unavailablePeers).thenReturn(new java.util.concurrent.ConcurrentHashMap())
Review Comment:
Same issue here: unavailablePeers expects ConcurrentHashMap[WorkerInfo, Long] (see Worker.scala), but new ConcurrentHashMap() will infer ConcurrentHashMap[Nothing, Nothing] in Scala and fail to compile. Instantiate with the correct generic types (or JavaUtils.newConcurrentHashMap).
```suggestion
when(mockWorker.unavailablePeers).thenReturn(
new java.util.concurrent.ConcurrentHashMap[WorkerInfo, Long]())
```
--
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
Re: [PR] [CELEBORN-2287] Split mode should be HARD_SPLIT when disk is full [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3653:
URL: https://github.com/apache/celeborn/pull/3653#discussion_r3056044524
##########
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:
Could you merge if branch for `workerPartitionSplitEnabled` condition as follows?
```
if (workerPartitionSplitEnabled) {
if (diskFull && diskFileInfo.getFileLength > partitionSplitMinimumSize) {
return StatusCode.HARD_SPLIT
}
if (isPrimary && diskFileInfo.getFileLength > fileWriter.getSplitThreshold) {
...
}
}
--
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
Re: [PR] [CELEBORN-2287] Split mode should be HARD_SPLIT when disk is full [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3653:
URL: https://github.com/apache/celeborn/pull/3653#discussion_r3056044524
##########
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:
Could you merge if branch for `workerPartitionSplitEnabled` condition as follows?
```
if (workerPartitionSplitEnabled) {
if (diskFull && diskFileInfo.getFileLength >= partitionSplitMinimumSize) {
return StatusCode.HARD_SPLIT
}
if (isPrimary && diskFileInfo.getFileLength >= fileWriter.getSplitThreshold) {
...
}
}
--
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
Re: [PR] [CELEBORN-2287] Split mode should be HARD_SPLIT when disk is full [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on PR #3653:
URL: https://github.com/apache/celeborn/pull/3653#issuecomment-4228113869
Merged to main(v0.7.0) and branch-0.6(v0.6.3).
--
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
Re: [PR] [CELEBORN-2287] Split mode should be HARD_SPLIT when disk is full [celeborn]
Posted by "zaynt4606 (via GitHub)" <gi...@apache.org>.
zaynt4606 commented on code in PR #3653:
URL: https://github.com/apache/celeborn/pull/3653#discussion_r3057803423
##########
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:
updated and add some UT.
--
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
Re: [PR] [CELEBORN-2287] Split mode should be hard when diskFull [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3653:
URL: https://github.com/apache/celeborn/pull/3653#discussion_r3056044524
##########
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:
Could you merge if branch for `workerPartitionSplitEnabled` condition?
--
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
Re: [PR] [CELEBORN-2287] Split mode should be HARD_SPLIT when disk is full [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3653:
URL: https://github.com/apache/celeborn/pull/3653#discussion_r3056044524
##########
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:
Could you merge if branch for `workerPartitionSplitEnabled` condition as follows?
```
if (workerPartitionSplitEnabled) {
if (diskFull && diskFileInfo.getFileLength > partitionSplitMinimumSize) {
return StatusCode.HARD_SPLIT
}
if (isPrimary && diskFileInfo.getFileLength > fileWriter.getSplitThreshold) {
...
}
}
--
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