henrib commented on code in PR #6704:
URL: https://github.com/apache/hive/pull/6704#discussion_r3860370534
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ServletSecurity.java:
##########
@@ -106,17 +116,113 @@ public static AuthType fromString(String type) {
private final Function<HttpServletRequest, List<String>> scopeProvider;
private SimpleJWTAuthenticator jwtAuthenticator = null;
private OAuth2Authenticator oAuth2Authenticator = null;
+ private final Cache<UgiKey, UserGroupInformation> proxyUserCache;
+
+ /**
+ * Cache key for a proxy {@link UserGroupInformation}. A proxy UGI is bound
to both the effective user it
+ * impersonates and the server login user acting as its real user, so both
participate in identity.
+ */
+ record UgiKey(String effectiveUser, String loginUser) {}
public ServletSecurity(AuthType authType, Configuration conf) {
this(authType, conf, null);
}
public ServletSecurity(AuthType authType, Configuration conf,
Function<HttpServletRequest, List<String>> scopeProvider) {
+ this(authType, conf, scopeProvider, ForkJoinPool.commonPool());
+ }
+
+ @VisibleForTesting
+ ServletSecurity(AuthType authType, Configuration conf,
+ Function<HttpServletRequest, List<String>> scopeProvider, Executor
cacheCleanupExecutor) {
this.conf = conf;
this.isSecurityEnabled = UserGroupInformation.isSecurityEnabled();
this.authType = authType;
this.scopeProvider = scopeProvider;
+ this.proxyUserCache = createCacheWithConfig(
+ MetastoreConf.getTimeVar(conf,
MetastoreConf.ConfVars.CATALOG_SERVLET_UGI_CACHE_EXPIRY, TimeUnit.MILLISECONDS),
+ MetastoreConf.getLongVar(conf,
MetastoreConf.ConfVars.CATALOG_SERVLET_UGI_CACHE_SIZE),
+ cacheCleanupExecutor);
+ }
+
+ /**
+ * Creates a UGI cache with the specified expiration time and maximum size.
+ *
+ * @param expirationMs Time in milliseconds after which entries expire due
to inactivity
+ * @param maxSize Maximum number of entries the cache can hold
+ * @param cacheCleanupExecutor executor on which removal-listener cleanup
({@link FileSystem#closeAllForUGI})
+ * runs; production uses {@link
ForkJoinPool#commonPool()} so cleanup stays off the
+ * request thread
+ * @return A configured Caffeine cache for UGI objects
+ */
+ private Cache<UgiKey, UserGroupInformation> createCacheWithConfig(long
expirationMs, long maxSize,
+ Executor cacheCleanupExecutor) {
+ // Note: eviction closes the UGI's FileSystems. If an entry is evicted
while a request is still inside doAs,
+ // that in-flight operation could see a "FileSystem closed" error. We
don't reference-count to prevent this;
Review Comment:
Good point. The design deliberately avoids reference counting: the eviction
policy is `expireAfterAccess`, so an entry cannot expire while it is still
being accessed. The code comment at the site documents the accepted tradeoff —
both the expiry window and the max cache size are expected to be kept
comfortably above the longest operation and peak concurrent distinct-user
count, making the scenario where an eviction races an active `doAs` a practical
non-issue. Adding reference counts would add significant complexity for a case
that requires a misconfigured cache (size or expiry too tight relative to
workload) to trigger.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ServletSecurity.java:
##########
@@ -106,17 +116,113 @@ public static AuthType fromString(String type) {
private final Function<HttpServletRequest, List<String>> scopeProvider;
private SimpleJWTAuthenticator jwtAuthenticator = null;
private OAuth2Authenticator oAuth2Authenticator = null;
+ private final Cache<UgiKey, UserGroupInformation> proxyUserCache;
+
+ /**
+ * Cache key for a proxy {@link UserGroupInformation}. A proxy UGI is bound
to both the effective user it
+ * impersonates and the server login user acting as its real user, so both
participate in identity.
+ */
+ record UgiKey(String effectiveUser, String loginUser) {}
public ServletSecurity(AuthType authType, Configuration conf) {
this(authType, conf, null);
}
public ServletSecurity(AuthType authType, Configuration conf,
Function<HttpServletRequest, List<String>> scopeProvider) {
+ this(authType, conf, scopeProvider, ForkJoinPool.commonPool());
+ }
+
+ @VisibleForTesting
+ ServletSecurity(AuthType authType, Configuration conf,
+ Function<HttpServletRequest, List<String>> scopeProvider, Executor
cacheCleanupExecutor) {
this.conf = conf;
this.isSecurityEnabled = UserGroupInformation.isSecurityEnabled();
this.authType = authType;
this.scopeProvider = scopeProvider;
+ this.proxyUserCache = createCacheWithConfig(
+ MetastoreConf.getTimeVar(conf,
MetastoreConf.ConfVars.CATALOG_SERVLET_UGI_CACHE_EXPIRY, TimeUnit.MILLISECONDS),
+ MetastoreConf.getLongVar(conf,
MetastoreConf.ConfVars.CATALOG_SERVLET_UGI_CACHE_SIZE),
+ cacheCleanupExecutor);
+ }
+
+ /**
+ * Creates a UGI cache with the specified expiration time and maximum size.
+ *
+ * @param expirationMs Time in milliseconds after which entries expire due
to inactivity
+ * @param maxSize Maximum number of entries the cache can hold
+ * @param cacheCleanupExecutor executor on which removal-listener cleanup
({@link FileSystem#closeAllForUGI})
+ * runs; production uses {@link
ForkJoinPool#commonPool()} so cleanup stays off the
+ * request thread
+ * @return A configured Caffeine cache for UGI objects
+ */
+ private Cache<UgiKey, UserGroupInformation> createCacheWithConfig(long
expirationMs, long maxSize,
+ Executor cacheCleanupExecutor) {
+ // Note: eviction closes the UGI's FileSystems. If an entry is evicted
while a request is still inside doAs,
+ // that in-flight operation could see a "FileSystem closed" error. We
don't reference-count to prevent this;
+ // instead we rely on generous margins: expiry is idle-based
(expireAfterAccess), and both the expiry window
+ // and maximumSize should be kept well above the longest operation / peak
concurrent distinct users.
+ RemovalListener<UgiKey, UserGroupInformation> cleanupListener =
+ (key, ugi, cause) -> {
+ if (ugi != null) {
+ try {
+ FileSystem.closeAllForUGI(ugi);
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Cleaned up FileSystem handles for evicted UGI: {}
(cause: {})",
+ ugi.getUserName(), cause);
+ }
+ } catch (IOException cleanupException) {
+ LOG.error("Failed to clean up FileSystem handles for evicted
UGI: {} (cause: {})",
+ ugi, cause, cleanupException);
+ }
+ }
+ };
+
+ Caffeine<UgiKey, UserGroupInformation> builder = Caffeine.<UgiKey,
UserGroupInformation>newBuilder()
+ .maximumSize(maxSize)
+ .executor(cacheCleanupExecutor)
+ .removalListener(cleanupListener);
+
+ if (expirationMs > 0) {
+ builder.expireAfterAccess(Duration.ofMillis(expirationMs))
Review Comment:
Addressed in the latest commit. The production constructor now schedules
`proxyUserCache::cleanUp()` on the dedicated maintenance executor at the
configured expiry interval, so entries accumulated during a burst are reaped
after they expire even when no subsequent traffic arrives to trigger Caffeine's
access-piggybacked maintenance.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ServletSecurity.java:
##########
@@ -106,17 +116,113 @@ public static AuthType fromString(String type) {
private final Function<HttpServletRequest, List<String>> scopeProvider;
private SimpleJWTAuthenticator jwtAuthenticator = null;
private OAuth2Authenticator oAuth2Authenticator = null;
+ private final Cache<UgiKey, UserGroupInformation> proxyUserCache;
+
+ /**
+ * Cache key for a proxy {@link UserGroupInformation}. A proxy UGI is bound
to both the effective user it
+ * impersonates and the server login user acting as its real user, so both
participate in identity.
+ */
+ record UgiKey(String effectiveUser, String loginUser) {}
public ServletSecurity(AuthType authType, Configuration conf) {
this(authType, conf, null);
}
public ServletSecurity(AuthType authType, Configuration conf,
Function<HttpServletRequest, List<String>> scopeProvider) {
+ this(authType, conf, scopeProvider, ForkJoinPool.commonPool());
+ }
+
+ @VisibleForTesting
+ ServletSecurity(AuthType authType, Configuration conf,
+ Function<HttpServletRequest, List<String>> scopeProvider, Executor
cacheCleanupExecutor) {
this.conf = conf;
this.isSecurityEnabled = UserGroupInformation.isSecurityEnabled();
this.authType = authType;
this.scopeProvider = scopeProvider;
+ this.proxyUserCache = createCacheWithConfig(
+ MetastoreConf.getTimeVar(conf,
MetastoreConf.ConfVars.CATALOG_SERVLET_UGI_CACHE_EXPIRY, TimeUnit.MILLISECONDS),
+ MetastoreConf.getLongVar(conf,
MetastoreConf.ConfVars.CATALOG_SERVLET_UGI_CACHE_SIZE),
+ cacheCleanupExecutor);
+ }
+
+ /**
+ * Creates a UGI cache with the specified expiration time and maximum size.
+ *
+ * @param expirationMs Time in milliseconds after which entries expire due
to inactivity
+ * @param maxSize Maximum number of entries the cache can hold
+ * @param cacheCleanupExecutor executor on which removal-listener cleanup
({@link FileSystem#closeAllForUGI})
+ * runs; production uses {@link
ForkJoinPool#commonPool()} so cleanup stays off the
+ * request thread
+ * @return A configured Caffeine cache for UGI objects
+ */
+ private Cache<UgiKey, UserGroupInformation> createCacheWithConfig(long
expirationMs, long maxSize,
+ Executor cacheCleanupExecutor) {
+ // Note: eviction closes the UGI's FileSystems. If an entry is evicted
while a request is still inside doAs,
+ // that in-flight operation could see a "FileSystem closed" error. We
don't reference-count to prevent this;
+ // instead we rely on generous margins: expiry is idle-based
(expireAfterAccess), and both the expiry window
+ // and maximumSize should be kept well above the longest operation / peak
concurrent distinct users.
+ RemovalListener<UgiKey, UserGroupInformation> cleanupListener =
+ (key, ugi, cause) -> {
+ if (ugi != null) {
+ try {
+ FileSystem.closeAllForUGI(ugi);
Review Comment:
Addressed in the latest commit. Replaced `ForkJoinPool.commonPool()` with a
dedicated `newSingleThreadScheduledExecutor` running a named daemon thread
(`"ugi-cache-cleanup"`), which isolates the blocking
`FileSystem.closeAllForUGI` calls from the JVM-wide shared pool. The same
executor also drives the periodic `cleanUp()` schedule added for the burst
scenario.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
