You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "FMX (via GitHub)" <gi...@apache.org> on 2023/05/22 09:02:35 UTC
[GitHub] [incubator-celeborn] FMX commented on a diff in pull request #1501: [CELEBORN-595] Rename and refactor the configuration doc.
FMX commented on code in PR #1501:
URL: https://github.com/apache/incubator-celeborn/pull/1501#discussion_r1200163663
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1309,16 +1309,73 @@ object CelebornConf extends Logging {
.booleanConf
.createWithDefault(false)
+ val RESERVE_SLOTS_RACKAWARE_ENABLED: ConfigEntry[Boolean] =
+ buildConf("celeborn.client.reserveSlots.rackware.enabled")
+ .categories("client")
+ .version("0.3.0")
+ .doc("Whether need to place different replicates on different racks when allocating slots.")
+ .booleanConf
+ .createWithDefault(false)
+
+ val RESERVE_SLOTS_MAX_RETRIES: ConfigEntry[Int] =
+ buildConf("celeborn.client.reserveSlots.maxRetries")
+ .withAlternative("celeborn.slots.reserve.maxRetries")
+ .withAlternative("rss.reserve.slots.max.retry")
+ .categories("client")
+ .version("0.2.0")
+ .doc("Max retry times for client to reserve slots.")
+ .intConf
+ .createWithDefault(3)
+
+ val RESERVE_SLOTS_RETRY_WAIT: ConfigEntry[Long] =
+ buildConf("celeborn.client.reserveSlots.retryWait")
+ .withAlternative("celeborn.slots.reserve.retryWait")
+ .withAlternative("rss.reserve.slots.retry.wait")
+ .categories("client")
+ .version("0.2.0")
+ .doc("Wait time before next retry if reserve slots failed.")
+ .timeConf(TimeUnit.MILLISECONDS)
+ .createWithDefaultString("3s")
+
+ val COMMIT_FILE_REQUEST_MAX_RETRY: ConfigEntry[Int] =
+ buildConf("celeborn.client.rpc.requestCommitFiles.maxRetries")
+ .withAlternative("celeborn.rpc.requestCommitFiles.maxRetries")
+ .categories("client")
+ .doc("Max retry times for requestCommitFiles RPC.")
+ .version("1.0.0")
Review Comment:
There might be some distance for us to release version 1.0.0.
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1370,23 +1430,181 @@ object CelebornConf extends Logging {
.createWithDefault(false)
val PUSH_TIMEOUT_CHECK_INTERVAL: ConfigEntry[Long] =
- buildConf("celeborn.push.timeoutCheck.interval")
+ buildConf("celeborn.client.push.timeoutCheck.interval")
+ .withAlternative("celeborn.push.timeoutCheck.interval")
.categories("client", "worker")
.doc("Interval for checking push data timeout.")
.version("0.3.0")
.timeConf(TimeUnit.MILLISECONDS)
.createWithDefaultString("5s")
val PUSH_TIMEOUT_CHECK_THREADS: ConfigEntry[Int] =
- buildConf("celeborn.push.timeoutCheck.threads")
+ buildConf("celeborn.client.push.timeoutCheck.threads")
+ .withAlternative("celeborn.push.timeoutCheck.threads")
.categories("client", "worker")
.doc("Threads num for checking push data timeout.")
.version("0.3.0")
.intConf
.createWithDefault(16)
+ val PUSH_STAGE_END_TIMEOUT: ConfigEntry[Long] =
+ buildConf("celeborn.client.push.stageEnd.timeout")
+ .withAlternative("celeborn.push.stageEnd.timeout")
+ .withAlternative("rss.stage.end.timeout")
+ .categories("client")
+ .doc(s"Timeout for waiting StageEnd. " +
+ s"During this process, there are `${COMMIT_FILE_REQUEST_MAX_RETRY.key}` times for retry opportunities for committing files" +
+ s"and 1 times for releasing slots request. User can customize this value according to your setting. " +
+ s"By default, the value is the max timeout value `${NETWORK_IO_CONNECTION_TIMEOUT.key}`.")
+ .version("0.2.0")
+ .fallbackConf(NETWORK_IO_CONNECTION_TIMEOUT)
+
+ val PUSH_LIMIT_STRATEGY: ConfigEntry[String] =
+ buildConf("celeborn.client.push.limit.strategy")
+ .withAlternative("celeborn.push.limit.strategy")
+ .categories("client")
+ .doc("The strategy used to control the push speed. " +
+ "Valid strategies are SIMPLE and SLOWSTART. the SLOWSTART strategy is usually cooperate with " +
+ "congest control mechanism in the worker side.")
+ .version("0.3.0")
+ .stringConf
+ .transform(_.toUpperCase(Locale.ROOT))
+ .checkValues(Set("SIMPLE", "SLOWSTART"))
+ .createWithDefaultString("SIMPLE")
+
+ val PUSH_SLOW_START_INITIAL_SLEEP_TIME: ConfigEntry[Long] =
+ buildConf("celeborn.client.push.slowStart.initialSleepTime")
+ .withAlternative("celeborn.push.slowStart.initialSleepTime")
+ .categories("client")
+ .version("0.3.0")
+ .doc(s"The initial sleep time if the current max in flight requests is 0")
+ .timeConf(TimeUnit.MILLISECONDS)
+ .createWithDefaultString("500ms")
+
+ val PUSH_SLOW_START_MAX_SLEEP_TIME: ConfigEntry[Long] =
+ buildConf("celeborn.client.push.slowStart.maxSleepTime")
+ .withAlternative("celeborn.push.slowStart.maxSleepTime")
+ .categories("client")
+ .version("0.3.0")
+ .doc(s"If ${PUSH_LIMIT_STRATEGY.key} is set to SLOWSTART, push side will " +
+ "take a sleep strategy for each batch of requests, this controls " +
+ "the max sleep time if the max in flight requests limit is 1 for a long time")
+ .timeConf(TimeUnit.MILLISECONDS)
+ .createWithDefaultString("2s")
+
+ val PUSH_DATA_TIMEOUT: ConfigEntry[Long] =
+ buildConf("celeborn.client.push.data.timeout")
+ .withAlternative("celeborn.push.data.timeout")
+ .withAlternative("rss.push.data.rpc.timeout")
+ .categories("client")
+ .version("0.2.0")
+ .doc(s"Timeout for a task to push data rpc message. This value should better be more than twice of `${PUSH_TIMEOUT_CHECK_INTERVAL.key}`")
+ .timeConf(TimeUnit.MILLISECONDS)
+ .checkValue(_ > 0, "celeborn.client.push.data.timeout must be positive!")
+ .createWithDefaultString("120s")
+
+ val TEST_PUSH_MASTER_DATA_TIMEOUT: ConfigEntry[Boolean] =
+ buildConf("celeborn.test.pushMasterDataTimeout")
Review Comment:
Shall we change this config name to celeborn.worker.test.pushMasterDataTimeout and celeborn.worker.test.pushMasterDataTimeout？Because there are two categories, I think we can keep the configs' names look constant.
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1370,23 +1430,181 @@ object CelebornConf extends Logging {
.createWithDefault(false)
val PUSH_TIMEOUT_CHECK_INTERVAL: ConfigEntry[Long] =
- buildConf("celeborn.push.timeoutCheck.interval")
+ buildConf("celeborn.client.push.timeoutCheck.interval")
+ .withAlternative("celeborn.push.timeoutCheck.interval")
.categories("client", "worker")
.doc("Interval for checking push data timeout.")
.version("0.3.0")
.timeConf(TimeUnit.MILLISECONDS)
.createWithDefaultString("5s")
val PUSH_TIMEOUT_CHECK_THREADS: ConfigEntry[Int] =
- buildConf("celeborn.push.timeoutCheck.threads")
+ buildConf("celeborn.client.push.timeoutCheck.threads")
+ .withAlternative("celeborn.push.timeoutCheck.threads")
.categories("client", "worker")
.doc("Threads num for checking push data timeout.")
.version("0.3.0")
.intConf
.createWithDefault(16)
+ val PUSH_STAGE_END_TIMEOUT: ConfigEntry[Long] =
+ buildConf("celeborn.client.push.stageEnd.timeout")
+ .withAlternative("celeborn.push.stageEnd.timeout")
+ .withAlternative("rss.stage.end.timeout")
+ .categories("client")
+ .doc(s"Timeout for waiting StageEnd. " +
+ s"During this process, there are `${COMMIT_FILE_REQUEST_MAX_RETRY.key}` times for retry opportunities for committing files" +
+ s"and 1 times for releasing slots request. User can customize this value according to your setting. " +
+ s"By default, the value is the max timeout value `${NETWORK_IO_CONNECTION_TIMEOUT.key}`.")
+ .version("0.2.0")
+ .fallbackConf(NETWORK_IO_CONNECTION_TIMEOUT)
+
+ val PUSH_LIMIT_STRATEGY: ConfigEntry[String] =
+ buildConf("celeborn.client.push.limit.strategy")
+ .withAlternative("celeborn.push.limit.strategy")
+ .categories("client")
+ .doc("The strategy used to control the push speed. " +
+ "Valid strategies are SIMPLE and SLOWSTART. the SLOWSTART strategy is usually cooperate with " +
+ "congest control mechanism in the worker side.")
+ .version("0.3.0")
+ .stringConf
+ .transform(_.toUpperCase(Locale.ROOT))
+ .checkValues(Set("SIMPLE", "SLOWSTART"))
+ .createWithDefaultString("SIMPLE")
+
+ val PUSH_SLOW_START_INITIAL_SLEEP_TIME: ConfigEntry[Long] =
+ buildConf("celeborn.client.push.slowStart.initialSleepTime")
+ .withAlternative("celeborn.push.slowStart.initialSleepTime")
+ .categories("client")
+ .version("0.3.0")
+ .doc(s"The initial sleep time if the current max in flight requests is 0")
+ .timeConf(TimeUnit.MILLISECONDS)
+ .createWithDefaultString("500ms")
+
+ val PUSH_SLOW_START_MAX_SLEEP_TIME: ConfigEntry[Long] =
+ buildConf("celeborn.client.push.slowStart.maxSleepTime")
+ .withAlternative("celeborn.push.slowStart.maxSleepTime")
+ .categories("client")
+ .version("0.3.0")
+ .doc(s"If ${PUSH_LIMIT_STRATEGY.key} is set to SLOWSTART, push side will " +
+ "take a sleep strategy for each batch of requests, this controls " +
+ "the max sleep time if the max in flight requests limit is 1 for a long time")
+ .timeConf(TimeUnit.MILLISECONDS)
+ .createWithDefaultString("2s")
+
+ val PUSH_DATA_TIMEOUT: ConfigEntry[Long] =
+ buildConf("celeborn.client.push.data.timeout")
+ .withAlternative("celeborn.push.data.timeout")
+ .withAlternative("rss.push.data.rpc.timeout")
+ .categories("client")
+ .version("0.2.0")
+ .doc(s"Timeout for a task to push data rpc message. This value should better be more than twice of `${PUSH_TIMEOUT_CHECK_INTERVAL.key}`")
+ .timeConf(TimeUnit.MILLISECONDS)
+ .checkValue(_ > 0, "celeborn.client.push.data.timeout must be positive!")
+ .createWithDefaultString("120s")
+
+ val TEST_PUSH_MASTER_DATA_TIMEOUT: ConfigEntry[Boolean] =
+ buildConf("celeborn.test.pushMasterDataTimeout")
+ .internal
+ .categories("worker", "test")
+ .version("0.2.0")
+ .doc("Whether to test push master data timeout")
+ .booleanConf
+ .createWithDefault(false)
+
+ val TEST_PUSH_SLAVE_DATA_TIMEOUT: ConfigEntry[Boolean] =
+ buildConf("celeborn.test.pushSlaveDataTimeout")
+ .internal
+ .categories("worker", "test")
Review Comment:
ditto.
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -3124,7 +3227,8 @@ object CelebornConf extends Logging {
.createWithDefault(10000)
val COLUMNAR_SHUFFLE_OFF_HEAP_ENABLED: ConfigEntry[Boolean] =
- buildConf("celeborn.columnar.offHeap.enabled")
+ buildConf("celeborn.columnar.shuffle.offHeap.enabled")
Review Comment:
If the config name's second portion is category, the name should be `celeborn.columnarShuffle.offHeap.enabled`.
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1992,7 +2239,8 @@ object CelebornConf extends Logging {
.createWithDefaultString("1000ms")
val WORKER_HEARTBEAT_TIMEOUT: ConfigEntry[Long] =
- buildConf("celeborn.worker.heartbeat.timeout")
+ buildConf("celeborn.master.worker.heartbeat.timeout")
Review Comment:
I think this name would be confusing. How about `celeborn.master.heartbeat.timeout`?
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1667,7 +1887,8 @@ object CelebornConf extends Logging {
.createWithDefaultString("4MB")
val HA_MASTER_RATIS_LOG_APPENDER_QUEUE_NUM_ELEMENTS: ConfigEntry[Int] =
- buildConf("celeborn.ha.master.ratis.raft.server.log.appender.buffer.element-limit")
+ buildConf("celeborn.ha.master.ha.ratis.raft.server.log.appender.buffer.element-limit")
Review Comment:
This config name is not semantic constant with others.
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -2251,183 +2505,45 @@ object CelebornConf extends Logging {
.timeConf(TimeUnit.MILLISECONDS)
.createWithDefaultString("10min")
- val METRICS_APP_TOP_DISK_USAGE_COUNT: ConfigEntry[Int] =
- buildConf("celeborn.metrics.app.topDiskUsage.count")
+ val MASTER_APP_TOP_DISK_USAGE_COUNT: ConfigEntry[Int] =
+ buildConf("celeborn.master.app.topDiskUsage.count")
Review Comment:
I think here need an extra alternative `celeborn.worker.app.topDiskUsage.count`.
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -2537,23 +2653,15 @@ object CelebornConf extends Logging {
.timeConf(TimeUnit.MILLISECONDS)
.createWithDefaultString("5s")
- val RESERVE_SLOTS_RACKAWARE_ENABLED: ConfigEntry[Boolean] =
- buildConf("celeborn.reserveSlots.rackware.enabled")
- .categories("client")
- .version("0.3.0")
- .doc("Whether need to place different replicates on different racks when allocating slots.")
- .booleanConf
- .createWithDefault(false)
-
val RESERVE_SLOTS_RPC_TIMEOUT: ConfigEntry[Long] =
- buildConf("celeborn.rpc.reserveSlots.askTimeout")
+ buildConf("celeborn.client.rpc.reserveSlots.askTimeout")
.categories("client")
.version("0.3.0")
.doc("Timeout for LifecycleManager request reserve slots.")
.fallbackConf(RPC_ASK_TIMEOUT)
val REGISTER_SHUFFLE_RPC_ASK_TIMEOUT: ConfigEntry[Long] =
- buildConf("celeborn.rpc.registerShuffle.askTimeout")
+ buildConf("celeborn.client.rpc.registerShuffle.askTimeout")
.categories("client")
.version("0.2.0")
Review Comment:
This config is changed so I think here needs an alternative.
--
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