saihemanth-cloudera commented on code in PR #6704:
URL: https://github.com/apache/hive/pull/6704#discussion_r3855853135
##########
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:
So we can potentially see failures for in-flight REST catalog operations.
How about we keep tracking of active users and deferring cleanup until the
last request exits, or using any another lifecycle model that never closes a
UGI currently in use?
##########
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:
_**This is edge case but worth thinking about**:_ A burst of distinct proxy
users followed by an idle period can keep the UGI/FileSystem resources alive
well past `metastore.catalog.servlet.ugi.cache.expiry`
Since the feature is specifically about idle cleanup, should we think about
adding a scheduler or a servlet-owned maintenance task that periodically calls
cleanUp()
##########
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:
This can perform blocking filesystem cleanup. Because we are invoking this
on `ForkJoinPool.commonPool()` running that on the JVM common pool risks
interfering with unrelated async work.
Should a small dedicated executor owned by ServletSecurity or the metastore
service would be a safer appraoch, with lifecycle shutdown?
--
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]
