This is an automated email from the ASF dual-hosted git repository.
rexxiong pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/celeborn.git
The following commit(s) were added to refs/heads/main by this push:
new fdc65fa3a [CELEBORN-1966] Added fix to get userinfo in celeborn
fdc65fa3a is described below
commit fdc65fa3a569f889109d9e04fffd0899abbf934c
Author: amandeeps.28 <[email protected]>
AuthorDate: Thu Apr 24 09:20:15 2025 +0800
[CELEBORN-1966] Added fix to get userinfo in celeborn
### What changes were proposed in this pull request?
Updated the DefaultIdentityProvider to return tenantId, name based on
configs provided by the user. Using the CelebornConf object used by
LifeCycleManager to get the tenantId and name.
### Why are the changes needed?
The tenant id and username passed by the user were not being used because
the DefaultIdentityProvider creates a new CelebornConf each time the provide
function is called. Due to this, the tenantid and username were always coming
as default. With these changes, we are using the CelebornConf object used by
the LifeCycleManager which includes the configs provided by the user.
### Does this PR introduce _any_ user-facing change?
NA
### How was this patch tested?
Added UTs and tested on staging setup
Closes #3214 from AmandeepSingh285/CELEBORN-1966.
Authored-by: amandeeps.28 <[email protected]>
Signed-off-by: Shuang <[email protected]>
---
.../common/identity/DefaultIdentityProvider.scala | 3 +-
.../identity/HadoopBasedIdentityProvider.scala | 4 +-
.../common/identity/IdentityProvider.scala | 6 ++-
.../org/apache/celeborn/common/util/Utils.scala | 4 +-
.../identity/DefaultIdentityProviderSuite.scala | 52 ++++++++++++++++++++++
.../apache/celeborn/common/util/UtilsSuite.scala | 6 ++-
.../config/DynamicConfigServiceFactory.java | 2 +-
7 files changed, 67 insertions(+), 10 deletions(-)
diff --git
a/common/src/main/scala/org/apache/celeborn/common/identity/DefaultIdentityProvider.scala
b/common/src/main/scala/org/apache/celeborn/common/identity/DefaultIdentityProvider.scala
index d9f1a10ad..35c9c0200 100644
---
a/common/src/main/scala/org/apache/celeborn/common/identity/DefaultIdentityProvider.scala
+++
b/common/src/main/scala/org/apache/celeborn/common/identity/DefaultIdentityProvider.scala
@@ -19,9 +19,8 @@ package org.apache.celeborn.common.identity
import org.apache.celeborn.common.CelebornConf
-class DefaultIdentityProvider extends IdentityProvider {
+class DefaultIdentityProvider(conf: CelebornConf) extends
IdentityProvider(conf) {
override def provide(): UserIdentifier = {
- val conf = new CelebornConf()
UserIdentifier(
conf.userSpecificTenant,
conf.userSpecificUserName)
diff --git
a/common/src/main/scala/org/apache/celeborn/common/identity/HadoopBasedIdentityProvider.scala
b/common/src/main/scala/org/apache/celeborn/common/identity/HadoopBasedIdentityProvider.scala
index acd8c0be4..fc5861e35 100644
---
a/common/src/main/scala/org/apache/celeborn/common/identity/HadoopBasedIdentityProvider.scala
+++
b/common/src/main/scala/org/apache/celeborn/common/identity/HadoopBasedIdentityProvider.scala
@@ -19,7 +19,9 @@ package org.apache.celeborn.common.identity
import org.apache.hadoop.security.UserGroupInformation
-class HadoopBasedIdentityProvider extends IdentityProvider {
+import org.apache.celeborn.common.CelebornConf
+
+class HadoopBasedIdentityProvider(conf: CelebornConf) extends
IdentityProvider(conf) {
override def provide(): UserIdentifier = {
UserIdentifier(
IdentityProvider.DEFAULT_TENANT_ID,
diff --git
a/common/src/main/scala/org/apache/celeborn/common/identity/IdentityProvider.scala
b/common/src/main/scala/org/apache/celeborn/common/identity/IdentityProvider.scala
index 5d4541840..ba78f9f35 100644
---
a/common/src/main/scala/org/apache/celeborn/common/identity/IdentityProvider.scala
+++
b/common/src/main/scala/org/apache/celeborn/common/identity/IdentityProvider.scala
@@ -21,7 +21,7 @@ import org.apache.celeborn.common.CelebornConf
import org.apache.celeborn.common.internal.Logging
import org.apache.celeborn.common.util.Utils
-abstract class IdentityProvider {
+abstract class IdentityProvider(conf: CelebornConf) {
def provide(): UserIdentifier
}
@@ -30,6 +30,8 @@ object IdentityProvider extends Logging {
val DEFAULT_USERNAME = "default"
def instantiate(conf: CelebornConf): IdentityProvider = {
- Utils.instantiate[IdentityProvider](conf.identityProviderClass)
+ val className = conf.identityProviderClass
+ logDebug(s"Creating instance of $className")
+ Utils.instantiateClassWithCelebornConf[IdentityProvider](className, conf)
}
}
diff --git a/common/src/main/scala/org/apache/celeborn/common/util/Utils.scala
b/common/src/main/scala/org/apache/celeborn/common/util/Utils.scala
index 2e959e23e..54811357e 100644
--- a/common/src/main/scala/org/apache/celeborn/common/util/Utils.scala
+++ b/common/src/main/scala/org/apache/celeborn/common/util/Utils.scala
@@ -613,7 +613,7 @@ object Utils extends Logging {
}
}
- def instantiateDynamicConfigStoreBackend[T](className: String, conf:
CelebornConf): T = {
+ def instantiateClassWithCelebornConf[T](className: String, conf:
CelebornConf): T = {
try {
DynConstructors.builder().impl(className, classOf[CelebornConf])
.build[T]()
@@ -621,7 +621,7 @@ object Utils extends Logging {
} catch {
case e: Throwable =>
throw new CelebornException(
- s"Failed to instantiate dynamic config store backend $className.",
+ s"Failed to instantiate class $className with celeborn conf.",
e)
}
}
diff --git
a/common/src/test/scala/org/apache/celeborn/common/identity/DefaultIdentityProviderSuite.scala
b/common/src/test/scala/org/apache/celeborn/common/identity/DefaultIdentityProviderSuite.scala
new file mode 100644
index 000000000..8fc3f2a04
--- /dev/null
+++
b/common/src/test/scala/org/apache/celeborn/common/identity/DefaultIdentityProviderSuite.scala
@@ -0,0 +1,52 @@
+/*
+ * 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.common.identity
+
+import org.apache.celeborn.CelebornFunSuite
+import org.apache.celeborn.common.CelebornConf
+
+class DefaultIdentityProviderSuite extends CelebornFunSuite {
+ test("provide() should use provided CelebornConf") {
+ val conf = new CelebornConf()
+
+ val TEST_TENANT_ID = "test-id"
+ val TEST_TENANT_NAME = "test-user"
+
+ conf.set(CelebornConf.USER_SPECIFIC_TENANT, TEST_TENANT_ID)
+ conf.set(CelebornConf.USER_SPECIFIC_USERNAME, TEST_TENANT_NAME)
+
+ val defaultIdentityProvider = new DefaultIdentityProvider(conf)
+ val userIdentifier = defaultIdentityProvider.provide()
+
+ assert(userIdentifier.tenantId == TEST_TENANT_ID)
+ assert(userIdentifier.name == TEST_TENANT_NAME)
+ }
+
+ test("provide() should use default CelebornConf if not provided") {
+ val conf = new CelebornConf()
+ val defaultIdentityProvider = new DefaultIdentityProvider(conf)
+
+ val DEFAULT_TENANT_ID = "default"
+ val DEFAULT_USERNAME = "default"
+
+ val userIdentifier = defaultIdentityProvider.provide()
+
+ assert(userIdentifier.tenantId == DEFAULT_TENANT_ID)
+ assert(userIdentifier.name == DEFAULT_USERNAME)
+ }
+}
diff --git
a/common/src/test/scala/org/apache/celeborn/common/util/UtilsSuite.scala
b/common/src/test/scala/org/apache/celeborn/common/util/UtilsSuite.scala
index d75006cf7..a9097e197 100644
--- a/common/src/test/scala/org/apache/celeborn/common/util/UtilsSuite.scala
+++ b/common/src/test/scala/org/apache/celeborn/common/util/UtilsSuite.scala
@@ -261,7 +261,9 @@ class UtilsSuite extends CelebornFunSuite {
test("test instantiate") {
val celebornConf = new CelebornConf()
-
assert(Utils.instantiate[DefaultIdentityProvider](celebornConf.identityProviderClass)
- .isInstanceOf[DefaultIdentityProvider])
+ val testInstance =
Utils.instantiateClassWithCelebornConf[DefaultIdentityProvider](
+ celebornConf.identityProviderClass,
+ celebornConf)
+ assert(testInstance.isInstanceOf[DefaultIdentityProvider])
}
}
diff --git
a/service/src/main/java/org/apache/celeborn/server/common/service/config/DynamicConfigServiceFactory.java
b/service/src/main/java/org/apache/celeborn/server/common/service/config/DynamicConfigServiceFactory.java
index 57049a994..ca9f67c1a 100644
---
a/service/src/main/java/org/apache/celeborn/server/common/service/config/DynamicConfigServiceFactory.java
+++
b/service/src/main/java/org/apache/celeborn/server/common/service/config/DynamicConfigServiceFactory.java
@@ -42,7 +42,7 @@ public class DynamicConfigServiceFactory {
break;
}
}
- _INSTANCE =
Utils.instantiateDynamicConfigStoreBackend(configStoreBackend, celebornConf);
+ _INSTANCE =
Utils.instantiateClassWithCelebornConf(configStoreBackend, celebornConf);
}
}
}
