mengw15 commented on code in PR #7539:
URL: https://github.com/apache/texera/pull/7539#discussion_r3773840829
##########
common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/IcebergCatalogInstance.scala:
##########
@@ -36,11 +39,34 @@ import scala.collection.mutable
* Only the REST catalog varies by warehouse; the hadoop and postgres
catalogs are warehouse-agnostic
* and ignore the warehouse argument.
*
- * Access is synchronized because the same JVM serves multiple warehouses
concurrently.
+ * The cache is bounded (#7290): per-user warehouses (#6870) make the set of
catalogs a
+ * long-lived JVM touches unbounded, and each REST catalog holds an HTTP
client. Entries
+ * fall out by size or idleness and are closed by the removal listener; the
next access
+ * simply rebuilds one. Callers must therefore resolve their catalog per use
instead of
+ * holding one across an execution (see IcebergDocument / IcebergTableWriter).
*/
-object IcebergCatalogInstance {
+object IcebergCatalogInstance extends LazyLogging {
- private val catalogs = mutable.Map.empty[String, Catalog]
+ // Sizing mirrors HuggingFaceModelResource's bounded-cache precedent:
generous enough
+ // that eviction never hits a warehouse in active use, small enough to bound
the JVM.
+ private val CatalogCacheMaxSize = 64L
+ private val CatalogCacheExpireAfterAccessMinutes = 60L
+
+ private val catalogs: Cache[String, Catalog] = CacheBuilder
+ .newBuilder()
+ .maximumSize(CatalogCacheMaxSize)
+ .expireAfterAccess(CatalogCacheExpireAfterAccessMinutes, TimeUnit.MINUTES)
+ .removalListener(new RemovalListener[String, Catalog] {
+ override def onRemoval(notification: RemovalNotification[String,
Catalog]): Unit =
+ notification.getValue match {
+ case closeable: AutoCloseable =>
+ Try(closeable.close()).failed.foreach(error =>
+ logger.warn(s"failed to close evicted catalog
'${notification.getKey}'", error)
Review Comment:
Follow-up in 239c4971e: the size-eviction half of this concern is now
retired entirely — only idle-expired entries are closed
(`RemovalCause.EXPIRED`). A size-evicted catalog is dropped un-closed: it lives
only as long as its in-flight operations (per-operation resolution bounds every
borrow), then GC reclaims it while the server's keepalive timeout severs its
idle connections. Overload therefore degrades into rebuild churn instead of
closing a catalog mid-operation; the only remaining close targets entries
untouched for 60 minutes.
--
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]
