[CELEBORN-1648] Refine AppUniqueId with UUID suffix #2810

chenkovsky · 2024-10-14T13:19:17Z

What changes were proposed in this pull request?

We can add randomUUID as an suffix to solve it

Why are the changes needed?

currently, we cannot guarantee application id is really unique. this may lead to data issue.

Does this PR introduce any user-facing change?

No

How was this patch tested?

test locally

SteNicholas · 2024-10-14T13:38:09Z

common/src/main/scala/org/apache/celeborn/common/util/Utils.scala

@@ -1305,4 +1305,8 @@ object Utils extends Logging {
        (key, value)
    }.asInstanceOf[Seq[(K, V)]]
  }
+
+  def appUniqueIdWithUUID(appUiqueId: String): String = {
+    appUiqueId + "-" + UUID.randomUUID()


Could you add config to allow user to add UUID suffix? IMO, not all cases should add UUID suffix. Right?

better to move appUniqueIdWithUUID(appUiqueId: String) to CelebornConf. Additionally, we can simplify the UUID by removing the dashes to create a shorter version, then we can use celebornConf.appUniqueIdWithUUIDSuffix

Suggested change

appUiqueId + "-" + UUID.randomUUID()

def appUniqueIdWithUUIDSuffix(appId: String): String = {

if (clientApplicationUUIDSuffixEnabled) {

appId + "-" + UUID.randomUUID().toString().replaceAll("-", "")

} else {

appId

}

}

SteNicholas · 2024-10-14T13:39:49Z

client-flink/common/src/main/java/org/apache/celeborn/plugin/flink/RemoteShuffleMaster.java

@@ -76,7 +77,9 @@ public void registerJob(JobShuffleContext context) {
    if (lifecycleManager == null) {
      synchronized (RemoteShuffleMaster.class) {
        if (lifecycleManager == null) {
-          celebornAppId = FlinkUtils.toCelebornAppId(lifecycleManagerTimestamp, jobID);
+          celebornAppId =
+              Utils.appUniqueIdWithUUID(


Why does flink app id need to add UUID suffix?

so you think there's no application id colision problem for flink?

Yes. cc @reswqa, @RexXiong.

Yes. cc @reswqa, @RexXiong.

Currently Flink Appid is already random and concat with current timestamp, but I think it's fine if we add flag for UUID suffix

I didn't expect that flink app id(JobId + timestamp) might conflict. Did you actually suffer from this problem.

RexXiong · 2024-10-15T01:19:13Z

common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala

@@ -5055,6 +5056,14 @@ object CelebornConf extends Logging {
      .checkValue(v => v > 0.0 && v <= 1.0, "Value must be between 0 and 1 (inclusive)")
      .createWithDefault(0.4)

+  val CLIENT_APPLICATION_UUID: ConfigEntry[Boolean] =
+    buildConf("celeborn.client.application.uuid.enabled")


perfer celeborn.client.application.uuidSuffix.enabled

RexXiong · 2024-10-15T01:20:18Z

common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala

@@ -905,6 +905,7 @@ class CelebornConf(loadDefaults: Boolean) extends Cloneable with Logging with Se
  def clientExcludeReplicaOnFailureEnabled: Boolean =
    get(CLIENT_EXCLUDE_PEER_WORKER_ON_FAILURE_ENABLED)
  def clientMrMaxPushData: Long = get(CLIENT_MR_PUSH_DATA_MAX)
+  def clientApplicationUUID: Boolean = get(CLIENT_APPLICATION_UUID)


clientApplicationUUID -> clientApplicationUUIDSuffixEnabled

RexXiong · 2024-10-15T01:30:21Z

common/src/main/scala/org/apache/celeborn/common/util/Utils.scala

@@ -1305,4 +1305,8 @@ object Utils extends Logging {
        (key, value)
    }.asInstanceOf[Seq[(K, V)]]
  }
+
+  def appUniqueIdWithUUID(appUiqueId: String): String = {
+    appUiqueId + "-" + UUID.randomUUID()


better to move appUniqueIdWithUUID(appUiqueId: String) to CelebornConf. Additionally, we can simplify the UUID by removing the dashes to create a shorter version, then we can use celebornConf.appUniqueIdWithUUIDSuffix

Suggested change

appUiqueId + "-" + UUID.randomUUID()

def appUniqueIdWithUUIDSuffix(appId: String): String = {

if (clientApplicationUUIDSuffixEnabled) {

appId + "-" + UUID.randomUUID().toString().replaceAll("-", "")

} else {

appId

}

}

SteNicholas · 2024-10-15T07:25:20Z

common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala

+    buildConf("celeborn.client.application.uuid.enabled")
+      .categories("client")
+      .version("0.6.0")
+      .doc("When `true`, add uuid suffix to application id")


Suggested change

.doc("When `true`, add uuid suffix to application id")

.doc("Whether to add UUID suffix for application id for unique. When `true`, add UUID suffix for unique application id.")

SteNicholas · 2024-10-15T07:27:03Z

client-flink/common/src/main/java/org/apache/celeborn/plugin/flink/RemoteShuffleMaster.java

@@ -76,12 +77,17 @@ public void registerJob(JobShuffleContext context) {
    if (lifecycleManager == null) {
      synchronized (RemoteShuffleMaster.class) {
        if (lifecycleManager == null) {
-          celebornAppId = FlinkUtils.toCelebornAppId(lifecycleManagerTimestamp, jobID);
-          LOG.info("CelebornAppId: {}", celebornAppId);
+          String applicationId = FlinkUtils.toCelebornAppId(lifecycleManagerTimestamp, jobID);


Please revert this change. Meanwhile, remove line 87~89.

Please revert this change. Meanwhile, remove line 87~89.

ok, let's revert it.
should we describe that the flag is only for spark and mr in doc ?

SteNicholas · 2024-10-15T07:27:17Z

...link-1.20/src/main/java/org/apache/celeborn/plugin/flink/tiered/CelebornTierMasterAgent.java

@@ -85,7 +86,12 @@ public void registerJob(JobID jobID, TierShuffleHandler tierShuffleHandler) {
    if (lifecycleManager == null) {
      synchronized (CelebornTierMasterAgent.class) {
        if (lifecycleManager == null) {
-          celebornAppId = FlinkUtils.toCelebornAppId(lifecycleManagerTimestamp, jobID);
+          String applicationId = FlinkUtils.toCelebornAppId(lifecycleManagerTimestamp, jobID);


SteNicholas reviewed Oct 14, 2024

View reviewed changes

RexXiong reviewed Oct 15, 2024

View reviewed changes

SteNicholas reviewed Oct 15, 2024

View reviewed changes

[CELEBORN-1648] Refine AppUniqueId with UUID suffix

ff6a9e9

chenkovsky force-pushed the feature/uuid_appid branch from 09fe17a to ff6a9e9 Compare October 15, 2024 11:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CELEBORN-1648] Refine AppUniqueId with UUID suffix #2810

[CELEBORN-1648] Refine AppUniqueId with UUID suffix #2810

chenkovsky commented Oct 14, 2024

SteNicholas Oct 14, 2024

RexXiong Oct 15, 2024

SteNicholas Oct 14, 2024 •

edited

Loading

chenkovsky Oct 14, 2024

SteNicholas Oct 14, 2024

RexXiong Oct 15, 2024

reswqa Oct 15, 2024

RexXiong Oct 15, 2024

RexXiong Oct 15, 2024

RexXiong Oct 15, 2024

SteNicholas Oct 15, 2024

SteNicholas Oct 15, 2024

chenkovsky Oct 15, 2024

SteNicholas Oct 15, 2024

-    appUiqueId + "-" + UUID.randomUUID()
+  def appUniqueIdWithUUIDSuffix(appId: String): String = {
+     if (clientApplicationUUIDSuffixEnabled) {
+        appId + "-" + UUID.randomUUID().toString().replaceAll("-", "")
+     } else {
+       appId
+     }
+   }

	.doc("When `true`, add uuid suffix to application id")
	.doc("Whether to add UUID suffix for application id for unique. When `true`, add UUID suffix for unique application id.")

[CELEBORN-1648] Refine AppUniqueId with UUID suffix #2810

Are you sure you want to change the base?

[CELEBORN-1648] Refine AppUniqueId with UUID suffix #2810

Conversation

chenkovsky commented Oct 14, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteNicholas Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteNicholas Oct 14, 2024 •

edited

Loading