[#5621] feat(core): support credential cache for Gravitino server #5995

FANNG1 · 2024-12-25T14:13:30Z

What changes were proposed in this pull request?

add credential cache for Gravitino server, not support for Iceberg rest server yet.

Why are the changes needed?

Fix: #5621

Does this PR introduce any user-facing change?

no

How was this patch tested?

testing in local env, get credential from Gravitino server and see whether it's fetched from remote or local cache

tengqm · 2024-12-26T00:03:44Z

core/src/main/java/org/apache/gravitino/credential/CatalogCredentialContext.java

+
+  @Override
+  public int hashCodeIgnoreUser() {
+    return 9999;


tengqm · 2024-12-26T00:05:08Z

core/src/main/java/org/apache/gravitino/credential/CatalogCredentialContext.java

+    if (o == null || !(o instanceof CatalogCredentialContext)) {
+      return false;
+    }
+    return true;


Do we need more checks before returning true?

tengqm · 2024-12-26T00:06:37Z

core/src/main/java/org/apache/gravitino/credential/CatalogCredentialManager.java

+  public Credential getCredential(CredentialCacheKey credentialCacheKey) {
+    String credentialType = credentialCacheKey.getCredentialType();
+    CredentialContext context = credentialCacheKey.getCredentialContext();
+    LOG.info("try get credential, credential type: {}, context: {}", credentialType, context);


Remove this in the final version?

tengqm · 2024-12-26T00:09:12Z

core/src/main/java/org/apache/gravitino/credential/CredentialCacheManager.java

+    // Set expire time after add a credential in the cache.
+    @Override
+    public long expireAfterCreate(
+        @NonNull T key, @NonNull Credential credential, long currentTime) {


Curious about the @NonNull annotation ...
Can we use it extensively for validation of null params?

tengqm · 2024-12-26T00:16:03Z

core/src/main/java/org/apache/gravitino/credential/CredentialUtils.java

+   * @param propertiesSuppliers The properties suppliers.
+   * @return A set of credential providers.
+   */
+  public static Set<String> getCredentialProvidersByOrder(


We are emphasizing "by order" because there will be a variant which is not by order?

not related

FANNG1 · 2024-12-26T06:53:00Z

@tengqm @jerryshao @yuqi1129 @orenccl , it's ready to review now, please help to review thanks

jerryshao · 2024-12-26T09:52:53Z

catalogs/catalog-common/src/main/java/org/apache/gravitino/credential/CredentialConstants.java

@@ -22,6 +22,8 @@
 public class CredentialConstants {
  public static final String CREDENTIAL_PROVIDER_TYPE = "credential-provider-type";
  public static final String CREDENTIAL_PROVIDERS = "credential-providers";
+  public static final String CREDENTIAL_CACHE_EXPIRE_IN_SECS = "credential-cache-expire-in-secs";


I feel that this property may not so useful, you can refer to Spark's token refresh config, set a ratio when to refresh the token.

I don't think we should refresh tokens periodic, the ideal implement may prefetch credentials according to the access rate of the credential, but this make cache complicated, I prefer to optimize it latter.

use credential-cache-expire-ratio

jerryshao · 2024-12-26T09:57:25Z

core/src/main/java/org/apache/gravitino/credential/CredentialCache.java

+  public void initialize(Map<String, String> catalogProperties) {
+    CredentialConfig credentialConfig = new CredentialConfig(catalogProperties);
+    long cache_size = credentialConfig.get(CredentialConfig.CREDENTIAL_CACHE_MAZ_SIZE);
+    long cache_expire_time = credentialConfig.get(CredentialConfig.CREDENTIAL_CACHE_EXPIRE_IN_SECS);


Why do you use this naming style?

orenccl · 2024-12-26T09:58:44Z

core/src/main/java/org/apache/gravitino/credential/config/CredentialConfig.java

+          .longConf()
+          .createWithDefault(DEFAULT_CREDENTIAL_CACHE_EXPIRE_IN_SECS);
+
+  public static final ConfigEntry<Long> CREDENTIAL_CACHE_MAZ_SIZE =


Should be MAX?

orenccl · 2024-12-26T10:23:45Z

core/src/test/java/org/apache/gravitino/credential/TestCredentialCacheKey.java

+    PathBasedCredentialContext context2 =
+        new PathBasedCredentialContext("user2", ImmutableSet.of("path1"), ImmutableSet.of("path2"));
+    PathBasedCredentialContext context3 =
+        new PathBasedCredentialContext("user3", ImmutableSet.of("path3"), ImmutableSet.of("path4"));


Would it be better to set user1 in context3 to test only different path?

Would it be better to name them context, contextWithDiffUser, and contextWithDiffPath?
This approach seems much clearer and eliminates the need for comments.
However, I’m fine with the current approach.

orenccl

Overall LGTM

FANNG1 · 2024-12-26T12:17:47Z

@jerryshao @orenccl @tengqm please help to review again

orenccl

Looks good from my side. the usage ratio seems much better!

tengqm · 2024-12-26T12:37:26Z

Overall LGTM.

jerryshao · 2024-12-26T12:50:40Z

core/src/main/java/org/apache/gravitino/credential/CredentialCache.java

+
+    // Set expire time after add a credential in the cache.
+    @Override
+    public long expireAfterCreate(T key, Credential credential, long currentTime) {


Is the currentTime here millisecond or nanosecond?

According the API it's nanosecond. currentTime – the current time, in nanoseconds

In test, the value of currentTime is odd, and according to the API, seems we should calculate the time by ourself.

Note: The currentTime is supplied by the configured Ticker and by default does not relate to system or wall-clock time. When calculating the duration based on a timestamp, the current time should be obtained independently.

I see, thanks.

jerryshao

LGTM.

FANNG1 · 2024-12-27T01:49:54Z

Thanks @jerryshao @tengqm @orenccl for reviewing

…er (apache#5995) ### What changes were proposed in this pull request? add credential cache for Gravitino server, not support for Iceberg rest server yet. ### Why are the changes needed? Fix: apache#4398 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? testing in local env, get credential from Gravitino server and see whether it's fetched from remote or local cache

FANNG1 marked this pull request as draft December 25, 2024 14:13

tengqm reviewed Dec 26, 2024

View reviewed changes

FANNG1 force-pushed the cache branch from f32da01 to 422dd7f Compare December 26, 2024 06:13

support credential cache

ef46e8e

FANNG1 force-pushed the cache branch from 377a6ec to ef46e8e Compare December 26, 2024 06:47

FANNG1 changed the title ~~[SIP] support credential cache~~ [#4398] feat(core): support credential cache for Gravitino server Dec 26, 2024

FANNG1 marked this pull request as ready for review December 26, 2024 07:03

jerryshao reviewed Dec 26, 2024

View reviewed changes

orenccl reviewed Dec 26, 2024

View reviewed changes

FANNG1 added 2 commits December 26, 2024 20:01

use ratio

2b44a23

use ratio

8c7efe3

orenccl approved these changes Dec 26, 2024

View reviewed changes

jerryshao reviewed Dec 26, 2024

View reviewed changes

jerryshao approved these changes Dec 26, 2024

View reviewed changes

FANNG1 self-assigned this Dec 27, 2024

FANNG1 merged commit d49e7eb into apache:main Dec 27, 2024
26 checks passed

FANNG1 changed the title ~~[#4398] feat(core): support credential cache for Gravitino server~~ [#5621] feat(core): support credential cache for Gravitino server Dec 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#5621] feat(core): support credential cache for Gravitino server #5995

[#5621] feat(core): support credential cache for Gravitino server #5995

FANNG1 commented Dec 25, 2024 •

edited

Loading

tengqm Dec 26, 2024

FANNG1 Dec 26, 2024

tengqm Dec 26, 2024

FANNG1 Dec 26, 2024

tengqm Dec 26, 2024

FANNG1 Dec 26, 2024

tengqm Dec 26, 2024

FANNG1 Dec 26, 2024

tengqm Dec 26, 2024

FANNG1 Dec 26, 2024

FANNG1 commented Dec 26, 2024

jerryshao Dec 26, 2024

FANNG1 Dec 26, 2024

FANNG1 Dec 26, 2024

jerryshao Dec 26, 2024

FANNG1 Dec 26, 2024

orenccl Dec 26, 2024

FANNG1 Dec 26, 2024

orenccl Dec 26, 2024

FANNG1 Dec 26, 2024

orenccl left a comment

FANNG1 commented Dec 26, 2024

orenccl left a comment

tengqm commented Dec 26, 2024

jerryshao Dec 26, 2024

FANNG1 Dec 26, 2024 •

edited

Loading

jerryshao Dec 26, 2024

jerryshao left a comment

FANNG1 commented Dec 27, 2024

[#5621] feat(core): support credential cache for Gravitino server #5995

[#5621] feat(core): support credential cache for Gravitino server #5995

Conversation

FANNG1 commented Dec 25, 2024 • edited Loading

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

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

FANNG1 commented Dec 26, 2024

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

orenccl left a comment

Choose a reason for hiding this comment

FANNG1 commented Dec 26, 2024

orenccl left a comment

Choose a reason for hiding this comment

tengqm commented Dec 26, 2024

Choose a reason for hiding this comment

FANNG1 Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao left a comment

Choose a reason for hiding this comment

FANNG1 commented Dec 27, 2024

FANNG1 commented Dec 25, 2024 •

edited

Loading

FANNG1 Dec 26, 2024 •

edited

Loading