yuqi1129 commented on PR #12852:
URL: https://github.com/apache/gravitino/pull/12852#issuecomment-5510161015
Thanks for the fix. Before going into the details, I want to ask one thing
about the premise, and suggest another place to fix it.
### Is `catalog-backend=memory` + auxiliary Iceberg REST a combination we
want to support?
The memory backend is basically a test/demo backend. Its data is gone when
the process restarts, and it is also gone when `CatalogManager` evicts the
catalog from `catalogCache`
(`expireAfterAccess(CATALOG_CACHE_EVICTION_INTERVAL_MS)`), because the borrowed
`InMemoryCatalog` is dropped together with the old `IcebergCatalogOperations`.
So before we add API for it: do we have a real use case for this
combination, or does it only show up in demos and ITs? If it is the latter, the
cheapest fix is to fail fast when a memory-backend catalog is used with the
auxiliary REST service. Today it returns a 500 anyway, so we would only be
replacing a confusing error with a clear one.
If we do want to support it, I think there is a simpler place to fix it.
### Suggestion: intern the memory catalog instead of passing the instance
around
The current approach hands a live Iceberg `Catalog` from `CatalogManager` to
the REST wrapper. That single decision brings in everything else in this PR:
the new `IcebergCatalogBackendProvider` interface, the `closeCatalogOnClose`
flag, the `default_catalog` alias resolution, and a dependency between two
caches that have different lifecycles.
Instead, `IcebergCatalogUtil.loadMemoryCatalog()` could return the same
instance for the same Gravitino catalog, i.e. keep them in a
`ConcurrentHashMap` instead of calling `new
MemoryCatalogWithMetadataLocationSupport()` every time.
This works across the isolated class loader: `IcebergCatalogUtil` is in
`org.apache.gravitino.iceberg.common.utils`, which matches no prefix in
`IsolatedClassLoader.isCatalogClass()`, so it is a shared class and is loaded
by the base class loader. There is only one copy of that static state in the
process, no matter how many catalog class loaders exist.
Then both sides just call `loadCatalogBackend()` as they do today and get
the same instance. We would not need:
- the new interface, or exposing a raw Iceberg `Catalog` on
`CatalogOperations`
- `closeCatalogOnClose`
- the `default_catalog` alias resolution
- the `auxMode && MEMORY && DynamicIcebergConfigProvider` condition, since
it works whichever side creates the catalog first
Two details to be careful about if you go this way:
1. Do not use `getCatalogBackendName()` as the map key.
`IcebergPropertiesUtils.getCatalogBackendName()` falls back to the backend type
string `"memory"` when `catalog-backend-name` is not set, so two memory
catalogs in the same metalake would collide.
`IcebergCatalogOperations.initialize()` already puts `catalog_uuid` into the
config (nothing reads it today), and
`DynamicIcebergConfigProvider.resolveProps()` has the Gravitino `Catalog`
object at hand, so a stable key is one line away on each side.
2. Remove the entry when the catalog is dropped, otherwise it leaks. It
should not be removed on idle eviction though, that would throw away the data.
### A problem in the current approach that this would also avoid
The listener registered in `IcebergCatalogWrapperManager` invalidates
`catalogWrapperCache` by `ident.name()`, but a client that connects without a
warehouse prefix is cached under `default_catalog`
(`IcebergRESTUtils.getCatalogName("")`). The two keys do not match, so the
`default_catalog` entry keeps the evicted backend. I wrote a small test for
this:
```
default_catalog still borrows the evicted backend: true
written into evicted backend : true
visible in current backend : false <- #12851 again
named catalog re-borrows new backend: true
```
`CatalogManager.catalogCache` uses `expireAfterAccess`, so this is not a
corner case. With a named prefix it works, but a client using the default
catalog silently falls back into the bug this PR fixes once the catalog is
evicted, until the REST side cache expires as well.
This can of course be fixed inside the current design (also invalidate
`default_catalog` when `ident.name()` equals
`configProvider.getDefaultCatalogName()`). I mention it mainly because looking
the catalog up by identity, instead of holding a reference to it, makes this
whole class of problem go away.
I have more comments on the tests, but let's agree on the direction first.
--
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]
