-
Notifications
You must be signed in to change notification settings - Fork 336
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#4014] feat(catalog-lakehouse-paimon): Support kerberos auth for Paimon HiveCatalog #5136
base: main
Are you sure you want to change the base?
Conversation
…n-hive-jdbc-kerberos
f3c76e4
to
c7240d4
Compare
|
||
private ClientPool<IMetaStoreClient, TException> resetPaimonHiveCachedClientPool() | ||
throws IllegalAccessException, NoSuchFieldException { | ||
final Field m = HiveCatalog.class.getDeclaredField("clients"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HiveCatalog‘s clients field may have alive threads. Do you need to close them first? cc @yuqi1129
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, the same goes for Iceberg Hive backend. The problem is that it's not so easy to close the pool and I have tried to do it in the Iceberg Hive backend but failed.
|
||
/** | ||
* Referred from Apache Paimon's CachedClientPool implementation | ||
* paimon-hive/paimon-hive-catalog/src/main/java/org/apache/paimon/hive/pool/CachedClientPool.java |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add this to the file LICENSE
.
throw new RuntimeException(e); | ||
} catch (InterruptedException e) { | ||
Thread.currentThread().interrupt(); | ||
throw new RuntimeException(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this right?
i will fix the ci exception and move it forward. |
|
||
private ClientPool<IMetaStoreClient, TException> resetPaimonHiveCachedClientPool() | ||
throws IllegalAccessException, NoSuchFieldException { | ||
final Field m = HiveCatalog.class.getDeclaredField("clients"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, the same goes for Iceberg Hive backend. The problem is that it's not so easy to close the pool and I have tried to do it in the Iceberg Hive backend but failed.
@@ -49,6 +52,13 @@ public AuthenticationConfig(Map<String, String> properties) { | |||
.stringConf() | |||
.createWithDefault("simple"); | |||
|
|||
public static final ConfigEntry<Boolean> ENABLE_IMPERSONATION_ENTRY = | |||
new ConfigBuilder(IMPERSONATION_ENABLE_KEY) | |||
.doc("Whether to enable impersonation for Iceberg catalog") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iceberg -> paimon
IMPERSONATION_ENABLE_KEY, | ||
PropertyEntry.booleanPropertyEntry( | ||
IMPERSONATION_ENABLE_KEY, | ||
"Whether to enable impersonation for the Iceberg catalog", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto.
this.conf = conf; | ||
this.clientPoolSize = options.get(CLIENT_POOL_SIZE); | ||
this.evictionInterval = options.get(CLIENT_POOL_CACHE_EVICTION_INTERVAL_MS); | ||
this.key = extractKey(clientClassName, options.get(CLIENT_POOL_CACHE_KEYS), conf); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the key is a constant value, consider the following scenario:
User1 access the Paimon catalog then a connection pool with the key was created, when user2 also wants to access the catalog, it will reuse the pool created by user1. Since connection contains user information, it will cause issues.
What changes were proposed in this pull request?
Support kerberos auth for Paimon HiveCatalog.
Why are the changes needed?
Fix: #4014
Does this PR introduce any user-facing change?
No.
How was this patch tested?
New ITs.