This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 1347c18ecb [MINOR] improvement(common): cache Credential ServiceLoader
lookup in CredentialFactory (#12423)
1347c18ecb is described below
commit 1347c18ecba9fe75290ee420b5465ed5c6a8d4d6
Author: YangJie <[email protected]>
AuthorDate: Wed Aug 12 19:52:48 2026 +0800
[MINOR] improvement(common): cache Credential ServiceLoader lookup in
CredentialFactory (#12423)
### What changes were proposed in this pull request?
`CredentialFactory.create` called `ServiceLoader.load(Credential.class)`
on every invocation and iterated/instantiated every registered
`Credential` implementation just to resolve one type -> class mapping.
This caches the scan once into an immutable `type -> class` map
(initialization-on-demand holder idiom) and turns `lookupCredential`
into a single map get.
Lookup stays case-insensitive and still throws `No credential found for:
<type>` (preserving the caller's original casing) on a miss;
duplicate-type detection moves to cache-build time.
### Why are the changes needed?
`Credential` is a fixed, built-in SPI (custom credentials are added via
`CredentialProvider`, not by registering new `Credential` services), so
the `ServiceLoader` result is stable for the JVM lifetime and does not
need to be rescanned — nor all providers re-instantiated — on every
lookup.
A JMH micro-benchmark shows the type lookup dropping from **~380 µs/op
to ~64 ns/op**:
```
Benchmark Mode Cnt Score
Error Units
CredentialLookupBenchmark.newCachedLookup avgt 3 64.064 ±
8.449 ns/op
CredentialLookupBenchmark.oldPerCallScan avgt 3 380491.264 ±
35292.735 ns/op
```
<details>
<summary>Benchmark used (not part of this PR — drop under
<code>core/src/jmh/java/org/apache/gravitino/credential/</code> and run
<code>./gradlew :core:jmh</code>)</summary>
`oldPerCallScan` replays the previous `ServiceLoader.load` +
stream-filter per call; `newCachedLookup` does a single `get` on the
pre-built map. Only the type-lookup step is measured; the shared
`newInstance` of the resolved credential is excluded, since it is
unchanged by this PR.
```java
package org.apache.gravitino.credential;
import java.util.HashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.ServiceLoader;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
import java.util.stream.StreamSupport;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.BenchmarkMode;
import org.openjdk.jmh.annotations.Mode;
import org.openjdk.jmh.annotations.OutputTimeUnit;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.infra.Blackhole;
@State(Scope.Benchmark)
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
public class CredentialLookupBenchmark {
private Map<String, Class<? extends Credential>> cachedClasses;
private String[] types;
@Setup
public void setup() {
cachedClasses = new HashMap<>();
for (Credential credential : ServiceLoader.load(Credential.class)) {
cachedClasses.put(
credential.credentialType().toLowerCase(Locale.ROOT),
credential.getClass());
}
types = cachedClasses.keySet().toArray(new String[0]);
}
@Benchmark
public void oldPerCallScan(Blackhole bh) {
for (String type : types) {
bh.consume(oldLookup(type));
}
}
@Benchmark
public void newCachedLookup(Blackhole bh) {
for (String type : types) {
bh.consume(cachedClasses.get(type.toLowerCase(Locale.ROOT)));
}
}
private static Class<? extends Credential> oldLookup(String
credentialType) {
ServiceLoader<Credential> serviceLoader =
ServiceLoader.load(Credential.class);
List<Class<? extends Credential>> credentials =
StreamSupport.stream(serviceLoader.spliterator(), false)
.filter(credential ->
credentialType.equalsIgnoreCase(credential.credentialType()))
.map(Credential::getClass)
.collect(Collectors.toList());
return credentials.get(0);
}
}
```
</details>
### Does this PR introduce _any_ user-facing change?
No. Internal factory optimization; lookup semantics (case-insensitive
resolution, `No credential found for: <type>` on miss) are unchanged.
### How was this patch tested?
Existing `TestCredentialFactory` cases plus new ones for unknown-type
rejection (including that the error message preserves the original
casing) and case-insensitive lookup. `./gradlew :common:test --tests
"org.apache.gravitino.credential.TestCredentialFactory"` and spotless
pass.
---
.../gravitino/credential/CredentialFactory.java | 49 +++++++++++++++-------
.../credential/TestCredentialFactory.java | 36 ++++++++++++++++
2 files changed, 70 insertions(+), 15 deletions(-)
diff --git
a/common/src/main/java/org/apache/gravitino/credential/CredentialFactory.java
b/common/src/main/java/org/apache/gravitino/credential/CredentialFactory.java
index 0ba46301b9..9698132956 100644
---
a/common/src/main/java/org/apache/gravitino/credential/CredentialFactory.java
+++
b/common/src/main/java/org/apache/gravitino/credential/CredentialFactory.java
@@ -19,15 +19,41 @@
package org.apache.gravitino.credential;
-import com.google.common.collect.Iterables;
-import com.google.common.collect.Streams;
-import java.util.List;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Locale;
import java.util.Map;
import java.util.ServiceLoader;
-import java.util.stream.Collectors;
/** Create a specific credential according to the credential information. */
public class CredentialFactory {
+
+ /**
+ * Holder for the lazily-initialized, immutable mapping from credential type
to its implementation
+ * class. {@link Credential} is a fixed, built-in SPI (all implementations
ship in the {@code api}
+ * and {@code common} modules on the same class loader as this factory;
custom credentials are
+ * added through {@link CredentialProvider}, not by registering new {@link
Credential} services),
+ * so the {@link ServiceLoader} result is stable for the lifetime of the JVM
and is safe to scan
+ * once and cache instead of on every {@link #create} call.
+ */
+ private static class CredentialClassesHolder {
+ private static final Map<String, Class<? extends Credential>>
CREDENTIAL_CLASSES =
+ loadCredentialClasses();
+
+ private static Map<String, Class<? extends Credential>>
loadCredentialClasses() {
+ Map<String, Class<? extends Credential>> classes = new HashMap<>();
+ for (Credential credential : ServiceLoader.load(Credential.class)) {
+ String type = credential.credentialType().toLowerCase(Locale.ROOT);
+ Class<? extends Credential> existing = classes.put(type,
credential.getClass());
+ if (existing != null) {
+ throw new RuntimeException(
+ "Multiple credentials found for: " +
credential.credentialType());
+ }
+ }
+ return Collections.unmodifiableMap(classes);
+ }
+ }
+
/**
* Creates a {@link Credential} instance based on the provided credential
type, information, and
* expiration time.
@@ -52,18 +78,11 @@ public class CredentialFactory {
}
private static Class<? extends Credential> lookupCredential(String
credentialType) {
- ServiceLoader<Credential> serviceLoader =
ServiceLoader.load(Credential.class);
- List<Class<? extends Credential>> credentials =
- Streams.stream(serviceLoader.iterator())
- .filter(credential ->
credentialType.equalsIgnoreCase(credential.credentialType()))
- .map(Credential::getClass)
- .collect(Collectors.toList());
- if (credentials.isEmpty()) {
+ Class<? extends Credential> credentialClz =
+
CredentialClassesHolder.CREDENTIAL_CLASSES.get(credentialType.toLowerCase(Locale.ROOT));
+ if (credentialClz == null) {
throw new RuntimeException("No credential found for: " + credentialType);
- } else if (credentials.size() > 1) {
- throw new RuntimeException("Multiple credential found for: " +
credentialType);
- } else {
- return Iterables.getOnlyElement(credentials);
}
+ return credentialClz;
}
}
diff --git
a/common/src/test/java/org/apache/gravitino/credential/TestCredentialFactory.java
b/common/src/test/java/org/apache/gravitino/credential/TestCredentialFactory.java
index 259949ae2b..eaa7402c2a 100644
---
a/common/src/test/java/org/apache/gravitino/credential/TestCredentialFactory.java
+++
b/common/src/test/java/org/apache/gravitino/credential/TestCredentialFactory.java
@@ -236,4 +236,40 @@ public class TestCredentialFactory {
Assertions.assertEquals(jdbcPassword, jdbcCredential.jdbcPassword());
Assertions.assertEquals(expireTime, jdbcCredential.expireTimeInMs());
}
+
+ @Test
+ void testUnknownCredentialType() {
+ RuntimeException e =
+ Assertions.assertThrows(
+ RuntimeException.class,
+ () -> CredentialFactory.create("no-such-credential",
ImmutableMap.of(), 0));
+ Assertions.assertEquals("No credential found for: no-such-credential",
e.getMessage());
+ }
+
+ @Test
+ void testUnknownCredentialTypeMessagePreservesOriginalCasing() {
+ // Lookup is case-insensitive, but the "not found" message must echo the
type as the caller
+ // supplied it, not the lower-cased lookup key.
+ RuntimeException e =
+ Assertions.assertThrows(
+ RuntimeException.class,
+ () -> CredentialFactory.create("No-Such-Credential",
ImmutableMap.of(), 0));
+ Assertions.assertEquals("No credential found for: No-Such-Credential",
e.getMessage());
+ }
+
+ @Test
+ void testCredentialTypeLookupIsCaseInsensitive() {
+ // The factory looks up the credential class by type case-insensitively; a
differently-cased
+ // type must resolve to the same implementation.
+ Credential credential =
+ CredentialFactory.create(
+
JdbcCredential.JDBC_CREDENTIAL_TYPE.toUpperCase(java.util.Locale.ROOT),
+ ImmutableMap.of(
+ JdbcCredential.GRAVITINO_JDBC_USER,
+ "test-user",
+ JdbcCredential.GRAVITINO_JDBC_PASSWORD,
+ "test-password"),
+ 0);
+ Assertions.assertInstanceOf(JdbcCredential.class, credential);
+ }
}
