MonkeyCanCode commented on PR #14590: URL: https://github.com/apache/iceberg/pull/14590#issuecomment-6610273359
> > * Do we really want to tie the pool to the Catalog? Catalogs are often recreated, and we don't handle lifecycle for them. > > I do share the same concern as you, so I'm hesitant on introducing closable catalogs that close the pool > > > * Shall we have our own lifecycle for the AuthSession threadpool? > > That makes sense to me and should fix the underlying issue Thanks @nastra @pvary. Agreed. I'll rescope this PR so the AuthSession eviction pool has its own lifecycle: a shared, lazily created pool like ThreadPools.authRefreshPool(), not owned by the catalog. I'll drop the Closeable changes to the Spark catalogs and CachingCatalog. Does that work for you? Also, the rest-metrics-reporter executor in RESTSessionCatalog has the same per-catalog lifecycle (one thread per catalog, only stopped on close). Should I make that shared as well, here or in a follow-up? -- 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]
