[GitHub] [incubator-celeborn] waitinfuture commented on a diff in pull request #1619: [CELEBORN-568] Support storage level selection

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

waitinfuture commented on code in PR #1619:
URL: https://github.com/apache/incubator-celeborn/pull/1619#discussion_r1242218577


##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -631,7 +646,8 @@ class CelebornConf(loadDefaults: Boolean) extends Cloneable with Logging with Se
   def workerCheckFileCleanTimeout: Long = get(WORKER_CHECK_FILE_CLEAN_TIMEOUT)
   def workerHeartbeatTimeout: Long = get(WORKER_HEARTBEAT_TIMEOUT)
   def workerReplicateThreads: Int = get(WORKER_REPLICATE_THREADS)
-  def workerCommitThreads: Int = get(WORKER_COMMIT_THREADS)
+  def workerCommitThreads: Int =
+    if (hasHDFSStorage) Math.max(256, get(WORKER_COMMIT_THREADS)) else get(WORKER_COMMIT_THREADS)

Review Comment:
   256 is quite big, do we really need so many commit threads?



##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/storage/StorageManager.scala:
##########
@@ -39,7 +39,7 @@ import org.apache.celeborn.common.identity.UserIdentifier
 import org.apache.celeborn.common.internal.Logging
 import org.apache.celeborn.common.meta.{DeviceInfo, DiskInfo, DiskStatus, FileInfo, TimeWindow}
 import org.apache.celeborn.common.metrics.source.AbstractSource
-import org.apache.celeborn.common.protocol.{PartitionLocation, PartitionSplitMode, PartitionType}
+import org.apache.celeborn.common.protocol.{PartitionLocation, PartitionSplitMode, PartitionType, StorageInfo}

Review Comment:
   StorageInfo seems unnecessary



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