yuqi1129 commented on code in PR #13058:
URL: https://github.com/apache/gravitino/pull/13058#discussion_r4004769088
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/GenericCatalogOperations.java:
##########
@@ -273,67 +322,138 @@ public Table alterTable(NameIdentifier ident,
TableChange... changes)
@Override
public boolean purgeTable(NameIdentifier ident) {
- boolean purged = tableOps(ident).purgeTable(ident);
- tableFormatCache.invalidate(ident);
- return purged;
+ return dropOrPurgeTable(ident, true /* purge */);
}
@Override
public boolean dropTable(NameIdentifier ident) throws
UnsupportedOperationException {
- boolean dropped = tableOps(ident).dropTable(ident);
- tableFormatCache.invalidate(ident);
- return dropped;
+ return dropOrPurgeTable(ident, false /* purge */);
}
- private String calculateTableLocation(
- Schema schema, NameIdentifier tableIdent, Map<String, String>
tableProperties) {
- String tableLocation =
- (String)
- propertiesMetadata
- .tablePropertiesMetadata()
- .getOrDefault(tableProperties, Table.PROPERTY_LOCATION);
- if (StringUtils.isNotBlank(tableLocation)) {
- return ensureTrailingSlash(tableLocation);
- }
+ /**
+ * Returns the cache mapping a table to its format, so that tests can assert
it is kept in step
+ * with the tables that exist.
+ *
+ * @return the table format cache
+ */
+ @VisibleForTesting
+ Cache<NameIdentifier, String> tableFormatCache() {
+ return tableFormatCache;
+ }
- String schemaLocation =
- schema.properties() == null ? null :
schema.properties().get(Schema.PROPERTY_LOCATION);
+ /**
+ * Drops or purges a table, and hands its location back to the {@link
TableLocationProvider}
+ * afterwards.
+ *
+ * <p>The table properties are read before the removal, because they carry
the location the
+ * provider has to hand back, and the unprovisioning itself happens after
the removal so that a
+ * provider never reclaims the storage of a table that is still there. A
provider failing to
+ * unprovision is logged at WARN rather than propagated: the table is
already gone at that point,
+ * so failing the request would report a drop that did in fact happen as
unsuccessful and invite a
+ * retry that cannot undo anything.
+ *
+ * @param ident the identifier of the table to drop
+ * @param purge whether to purge the table instead of dropping it
+ * @return true if the table was dropped, false if it did not exist
+ */
+ private boolean dropOrPurgeTable(NameIdentifier ident, boolean purge) {
+ return dropOrPurgeTable(
+ ident, purge,
loadSchema(NameIdentifier.of(ident.namespace().levels())));
+ }
- // If we do not set location in table properties, and schema location is
set, use schema
- // location as the base path.
- if (StringUtils.isNotBlank(schemaLocation)) {
- return ensureTrailingSlash(schemaLocation) + tableIdent.name() + SLASH;
+ /**
+ * Drops or purges a table, resolving its parent schema through the given
supplier.
+ *
+ * <p>The schema is passed in rather than loaded here so that a cascading
schema drop, where every
+ * table shares one parent, loads it once instead of once per table. It
stays eager: the context
+ * is built in full before the table is removed, so that a store read
failing fails the request
+ * while the table is still there, rather than from inside a callback where
it could only be
+ * reported as a provider failure it is not.
+ *
+ * @param ident the identifier of the table to drop
+ * @param purge whether to purge the table instead of dropping it
+ * @param schema the table's parent schema
+ * @return true if the table was dropped, false if it did not exist
+ */
+ private boolean dropOrPurgeTable(NameIdentifier ident, boolean purge, Schema
schema) {
+ Map<String, String> tableProperties;
+ try {
+ tableProperties = store.get(ident, TABLE,
TableEntity.class).properties();
+ } catch (NoSuchEntityException e) {
Review Comment:
Compatibility note: this is where `DELETE .../tables/x` (and `?purge=true`)
for a missing table changes from `404` to `200 {"dropped": false}` — the old
`tableOps(ident)` path mapped `NoSuchEntityException` to
`NoSuchTableException`. Aligning with `TableCatalog.dropTable` and the other
catalogs is the right direction, but it is a wire-level change for existing
clients, so it should be called out in `docs/lakehouse-generic-catalog.md` /
the release notes rather than only in the PR body.
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/GenericCatalogOperations.java:
##########
@@ -233,25 +269,38 @@ public Table createTable(
Index[] indexes)
throws NoSuchSchemaException, TableAlreadyExistsException {
Schema schema = loadSchema(NameIdentifier.of(ident.namespace().levels()));
- String tableLocation = calculateTableLocation(schema, ident, properties);
String format = properties.getOrDefault(Table.PROPERTY_TABLE_FORMAT, null);
Preconditions.checkArgument(
format != null, "Table format must be specified in table properties");
format = format.toLowerCase(Locale.ROOT);
- Map<String, String> newProperties = Maps.newHashMap(properties);
- newProperties.put(Table.PROPERTY_LOCATION, tableLocation);
- newProperties.put(Table.PROPERTY_TABLE_FORMAT, format);
-
// Get the table operations for the specified table format.
Supplier<ManagedTableOperations> tableOpsSupplier =
tableOpsCache.get(format);
Preconditions.checkArgument(tableOpsSupplier != null, "Unsupported table
format: %s", format);
ManagedTableOperations tableOps =
configureTableOps(tableOpsSupplier.get());
+ // The provider is consulted only once the request is known to be one this
catalog can serve,
+ // and only when the catalog is the one choosing the location. A provider
that allocates real
+ // storage has no compensating callback, so every check that can be made
before asking it for a
+ // location is one reservation it does not have to reclaim later.
+ String suppliedLocation = properties.get(Table.PROPERTY_LOCATION);
+ boolean provisioned = StringUtils.isBlank(suppliedLocation);
Review Comment:
**SPI contract (worth settling before this becomes public API).** A
non-blank `location` skips the provider completely, so a provider can never
see, validate or reject a caller-chosen path. The PR's own motivation lists "a
client passing an arbitrary path that bypasses the policy" as one of the two
poor workarounds this SPI is meant to replace, but the implementation keeps
exactly that bypass and delegates it to "reject it before it reaches the
catalog".
Alternative: always call `provisionTableLocation`, with the supplied value
visible through `context.tableProperties().get("location")`.
`DefaultTableLocationProvider` already returns it verbatim as its first branch
— that *is* the old chain, so the default behaviour does not change — and an
allocating provider decides for itself whether to honour, reject or rewrite it
(the contract can say plainly that a non-blank location means the caller has
already chosen a path and the data is usually already there). Cost: one `if` in
allocating providers. Benefit: policy providers become possible, the
`provisioned` special case here goes away, and the first branch of the default
provider stops being dead code on the catalog path (it can never be reached
today).
If you'd rather keep the current rule, please at least remove that dead
branch from `DefaultTableLocationProvider.provisionTableLocation` and state in
the SPI javadoc that a provider is never consulted for a supplied location.
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/TableLocationProvider.java:
##########
@@ -0,0 +1,264 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.catalog.lakehouse.generic;
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.util.Map;
+
+/**
+ * A pluggable strategy for deciding where the data of a newly created table
lives.
+ *
+ * <p>Implementations are discovered through Java's {@link
java.util.ServiceLoader} and selected by
+ * {@link #name()} using the {@code table-location-provider} catalog property.
The built-in {@link
+ * DefaultTableLocationProvider} derives the location from the table, schema
and catalog {@code
+ * location} properties; deployments that allocate storage through an external
service can register
+ * their own implementation instead.
+ *
+ * <p>An instance is created per catalog, {@link #initialize(Map)} is called
once before any
+ * provisioning, and {@link #close()} is called when the catalog is closed.
Implementations must be
+ * thread-safe: {@link #provisionTableLocation(TableLocationContext)}, {@link
+ * #unprovisionTableLocation(TableLocationContext)} and {@link
+ * #releaseUnusedLocation(TableLocationContext)} are all called concurrently
by table requests.
+ *
+ * <p>The catalog does not fence in-flight requests against {@link #close()},
so a request that
+ * started before the catalog was closed can reach any of the three callbacks
afterwards. An
+ * implementation is not asked to keep working across a close; it is asked to
fail cleanly rather
+ * than corrupt anything, which is what a closed client throwing does on its
own. On the drop and
+ * release paths the failure is logged at WARN and nothing else happens; on
the provisioning path it
+ * fails the table creation, which is the right answer for a catalog that is
shutting down.
+ *
+ * <p>Implementations must satisfy two constraints imposed by the {@link
java.util.ServiceLoader}
+ * based discovery:
+ *
+ * <ul>
+ * <li>They must have a public no-argument constructor that is cheap, does
not throw and acquires
+ * nothing. Every provider registered on the classpath is instantiated
before the one matching
+ * the catalog property is selected, so a heavy constructor slows the
initialization of every
+ * catalog, including those using the built-in provider. One that throws
is logged and skipped
+ * rather than failing the lookup, which costs that provider the ability
to be selected at
+ * all. The instances that were not selected are then discarded without
{@link #close()} being
+ * called on them, so anything a constructor acquires is leaked once per
catalog creation.
+ * Connection pools, remote clients and any other expensive setup belong
in {@link
+ * #initialize(Map)}.
+ * <li>{@link #name()} must be unique across the classpath, and must not be
{@value
+ * DefaultTableLocationProvider#NAME}, which is reserved by {@link
+ * DefaultTableLocationProvider}. If two providers share a name, every
catalog selecting that
+ * name fails to initialize. A provider whose constructor or {@link
#name()} throws is logged
+ * and skipped rather than failing the lookup, so a provider that is
broken at runtime does
+ * not stop catalogs that named a different one from starting. A
services file naming a class
+ * that cannot be loaded at all still fails the lookup.
+ * </ul>
+ *
+ * <p><b>Known limitations.</b> Four of them, and they all point the same way:
a provider that
+ * manages real storage needs its own reconciliation against the catalog and
cannot treat the
+ * callbacks here as a complete record of what it handed out.
+ *
+ * <ul>
+ * <li>{@code location} is a mutable table property, so {@code
alterTable(setProperty("location",
+ * ...))} repoints a table without this provider being told. The old
location is never
+ * unprovisioned and the new one never went through this provider.
+ * <li>{@link #provisionTableLocation(TableLocationContext)} is called
before the table is
+ * actually created, so a creation that fails afterwards -- a table that
already exists, or a
+ * failure inside the table format itself -- leaves a location
provisioned for a table that
+ * does not exist. There is no compensating unprovision, deliberately: a
table format that
+ * fails partway through creation may already have written to the
location, and calling {@link
+ * #unprovisionTableLocation(TableLocationContext)} would then tell the
provider it is free to
+ * reclaim a path that has data on it. Leaking an unused path is the
safer of the two
+ * failures, and doing better would need the format to report whether it
touched storage
+ * before failing, which this interface cannot express. Every check this
catalog can make on
+ * its own is made before the provider is consulted, so the cases that
remain are the ones
+ * only the table format can detect.
+ * <li>A table format that drops a table through its own internals, rather
than through the
+ * catalog, does not trigger {@link
#unprovisionTableLocation(TableLocationContext)}. Lance's
+ * {@code OVERWRITE} creation mode does this: it drops the existing
table and creates a new
+ * one, so the old location is never handed back. A format-internal drop
is not visible to the
+ * catalog, so this cannot be closed from here.
+ * <li>{@code alterTable(rename(...))} changes a table's identity without
telling this provider,
+ * and without moving any data. A provider deriving the path from the
table name is left with
+ * a path that no longer matches the name, which is cosmetic. A provider
that books
+ * allocations against {@code (schema, table)} loses the table
altogether: the drop that
+ * follows arrives under the new name, and the allocation booked under
the old one is never
+ * handed back. Such a provider has to reconcile renames out of band, or
the deployment has to
+ * forbid renaming tables in this catalog.
+ * </ul>
+ */
+public interface TableLocationProvider extends Closeable {
+
+ /**
+ * Returns the name identifying this provider. The value is matched
case-insensitively against the
+ * {@code table-location-provider} catalog property to select a provider.
+ *
+ * @return the provider name, never null or blank
+ */
+ String name();
+
+ /**
+ * Initializes the provider with the properties of the catalog it belongs
to. Called exactly once,
+ * before any call to {@link #provisionTableLocation(TableLocationContext)}.
+ *
+ * <p>The default implementation does nothing, because a provider that
derives the location purely
+ * from {@link TableLocationContext} has nothing to prepare. Providers that
hold a remote client,
+ * a connection or any state that must outlive a single table creation must
override it, and
+ * release those resources in {@link #close()}.
+ *
+ * <p>Throwing from here fails the initialization of the catalog. The
instance is closed before
+ * the failure propagates, so a provider that acquired part of its resources
before giving up
+ * still gets to release them in {@link #close()}.
+ *
+ * @param catalogProperties the properties of the catalog owning this
provider, never null and
+ * never modified after this call
+ */
+ default void initialize(Map<String, String> catalogProperties) {}
+
+ /**
+ * Provisions the location for the table that is being created.
+ *
+ * <p>The returned location is stored verbatim in the table's {@code
location} property. It must
+ * be non-blank; the caller rejects the table creation with an {@link
IllegalArgumentException}
+ * otherwise. Nothing else is required of it: the shape of the path belongs
to the provider,
+ * nothing downstream appends to the location, and storing it unchanged is
what lets a provider
+ * unprovisioning it later match the string it handed out.
+ *
+ * <p>A request that carries its own {@code location} never reaches this
method; the supplied
+ * value is stored, normalized only with a trailing slash. In this catalog a
caller supplies a
+ * location mostly because the data is already there -- an external Delta
table, or a Lance
+ * registration -- and allocating a fresh empty path for one of those would
orphan the caller's
+ * data while still reporting success. The catalog cannot tell those
requests apart from a caller
+ * merely overriding placement, so it keeps the supplied location in both
cases, which is also
+ * what it has always done. A deployment that wants allocation to be
mandatory has to reject a
+ * caller-supplied location before it reaches the catalog; a provider cannot
enforce it, because
+ * it is not called.
+ *
+ * <p>Everything else reaches this method, including an external table that
carries no location.
+ * Whether a table is external can be read from the {@code external} entry
of {@link
+ * TableLocationContext#tableProperties()}.
+ *
+ * @param context the table being created and the context needed to derive
its location
+ * @return the provisioned table location, never null or blank
+ * @throws IllegalArgumentException if no location can be derived from the
given context
+ */
+ String provisionTableLocation(TableLocationContext context);
+
+ /**
+ * Unprovisions the location of a table that has been dropped, so that a
provider allocating
+ * storage through an external service can hand it back instead of leaking
it. The location to
+ * hand back is the {@code location} entry of {@link
TableLocationContext#tableProperties()}.
+ *
+ * <p>There is deliberately no default implementation. A provider allocating
from an external
+ * system has to state what happens when the table goes away, and a provider
deriving the path
+ * from configuration writes an empty body and says so; an inherited empty
body would let the
+ * second answer be given by accident.
+ *
+ * <p>It is called <em>after</em> the table metadata, and the underlying
data of a managed table,
+ * have been removed, so throwing does not roll the drop back: the table is
gone either way. The
+ * failure is logged at WARN and the drop still reports success. Dropping a
schema with cascade
+ * unprovisions the location of every table it contains, one by one.
+ *
+ * <p>It is <em>not</em> called for external tables. The catalog does not
own their data -- the
+ * table formats leave the dataset in place on drop -- so asking a provider
to hand the location
+ * back would invite it to delete exactly the data the catalog just promised
not to touch. A leak
+ * is recoverable and a deletion is not, so the callback is skipped. {@link
+ * TableLocationContext#isExternal()} reports the same flag on the paths
where it is called.
+ *
+ * <p>The external flag is an approximation of the rule this callback
actually wants, which is
+ * "hand back only what was handed out", and two cases stay asymmetric under
it. An external table
+ * created without a location <em>does</em> get one provisioned -- both
table formats check the
+ * location after the catalog has filled it in -- and skipping leaks that
one. A table that is not
+ * external but whose creation carried its own location was never
provisioned, yet is still
+ * unprovisioned here, so the provider is asked about a path it never
issued. Distinguishing them
+ * exactly would need the catalog to record, per table, whether it
provisioned the location, which
+ * it does not do today. Both cases are why an implementation reclaiming
real storage needs its
+ * own reconciliation, and why this method must tolerate a location it does
not recognize.
+ *
+ * <p>There is no purge flag in the context, because for the tables this
catalog manages there is
+ * nothing to distinguish: {@code ManagedTableOperations.purgeTable}
delegates straight to {@code
+ * dropTable}, so the two paths remove exactly the same things. The signal
that matters is {@code
Review Comment:
This sentence is not true for the Lance format in this very catalog:
`LanceTableOperations.purgeTable` overrides `purgeTable` and deletes the
external dataset that `dropTable` leaves in place. A provider author reading
this will not know that a purge can destroy data under a location the provider
is not told about (see the comment on `dropOrPurgeTable`). Please reword, and
ideally expose purge vs drop on the context.
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/GenericCatalogOperations.java:
##########
@@ -273,67 +322,138 @@ public Table alterTable(NameIdentifier ident,
TableChange... changes)
@Override
public boolean purgeTable(NameIdentifier ident) {
- boolean purged = tableOps(ident).purgeTable(ident);
- tableFormatCache.invalidate(ident);
- return purged;
+ return dropOrPurgeTable(ident, true /* purge */);
}
@Override
public boolean dropTable(NameIdentifier ident) throws
UnsupportedOperationException {
- boolean dropped = tableOps(ident).dropTable(ident);
- tableFormatCache.invalidate(ident);
- return dropped;
+ return dropOrPurgeTable(ident, false /* purge */);
}
- private String calculateTableLocation(
- Schema schema, NameIdentifier tableIdent, Map<String, String>
tableProperties) {
- String tableLocation =
- (String)
- propertiesMetadata
- .tablePropertiesMetadata()
- .getOrDefault(tableProperties, Table.PROPERTY_LOCATION);
- if (StringUtils.isNotBlank(tableLocation)) {
- return ensureTrailingSlash(tableLocation);
- }
+ /**
+ * Returns the cache mapping a table to its format, so that tests can assert
it is kept in step
+ * with the tables that exist.
+ *
+ * @return the table format cache
+ */
+ @VisibleForTesting
+ Cache<NameIdentifier, String> tableFormatCache() {
+ return tableFormatCache;
+ }
- String schemaLocation =
- schema.properties() == null ? null :
schema.properties().get(Schema.PROPERTY_LOCATION);
+ /**
+ * Drops or purges a table, and hands its location back to the {@link
TableLocationProvider}
+ * afterwards.
+ *
+ * <p>The table properties are read before the removal, because they carry
the location the
+ * provider has to hand back, and the unprovisioning itself happens after
the removal so that a
+ * provider never reclaims the storage of a table that is still there. A
provider failing to
+ * unprovision is logged at WARN rather than propagated: the table is
already gone at that point,
+ * so failing the request would report a drop that did in fact happen as
unsuccessful and invite a
+ * retry that cannot undo anything.
+ *
+ * @param ident the identifier of the table to drop
+ * @param purge whether to purge the table instead of dropping it
+ * @return true if the table was dropped, false if it did not exist
+ */
+ private boolean dropOrPurgeTable(NameIdentifier ident, boolean purge) {
+ return dropOrPurgeTable(
+ ident, purge,
loadSchema(NameIdentifier.of(ident.namespace().levels())));
+ }
- // If we do not set location in table properties, and schema location is
set, use schema
- // location as the base path.
- if (StringUtils.isNotBlank(schemaLocation)) {
- return ensureTrailingSlash(schemaLocation) + tableIdent.name() + SLASH;
+ /**
+ * Drops or purges a table, resolving its parent schema through the given
supplier.
+ *
+ * <p>The schema is passed in rather than loaded here so that a cascading
schema drop, where every
+ * table shares one parent, loads it once instead of once per table. It
stays eager: the context
+ * is built in full before the table is removed, so that a store read
failing fails the request
+ * while the table is still there, rather than from inside a callback where
it could only be
+ * reported as a provider failure it is not.
+ *
+ * @param ident the identifier of the table to drop
+ * @param purge whether to purge the table instead of dropping it
+ * @param schema the table's parent schema
+ * @return true if the table was dropped, false if it did not exist
+ */
+ private boolean dropOrPurgeTable(NameIdentifier ident, boolean purge, Schema
schema) {
+ Map<String, String> tableProperties;
+ try {
+ tableProperties = store.get(ident, TABLE,
TableEntity.class).properties();
+ } catch (NoSuchEntityException e) {
+ return false;
+ } catch (IOException e) {
+ throw new RuntimeException(
+ String.format("Failed to load table %s before dropping it", ident),
e);
}
- // If the schema location is not set, use catalog lakehouse dir as the
base path. Or else, throw
- // an exception.
- if (catalogLocation.isEmpty()) {
- throw new IllegalArgumentException(
- "'location' property is neither set in table properties "
- + "nor in schema properties, and no location is set in catalog
properties either. "
- + "Please set the 'location' in either of them to create the
table "
- + tableIdent);
+ // Built entirely before the drop, so that a store read failing here fails
the request while
+ // the table is still there, rather than after it is gone where it could
only be reported as a
+ // provider failure it is not.
+ TableLocationContext context =
+ TableLocationContext.builder()
+ .withTableIdentifier(ident)
+ .withTableProperties(tableProperties)
+ .withSchema(schema)
+ .build();
+
+ // The properties just read are handed on rather than left to be read
again: resolving the
+ // table format is a second store read for the very same entity whenever
the format cache is
+ // cold, which for a drop it usually is.
+ ManagedTableOperations tableOps = tableOps(ident, tableProperties);
+ boolean dropped = purge ? tableOps.purgeTable(ident) :
tableOps.dropTable(ident);
Review Comment:
If the format throws *after* it has removed the metadata (e.g.
`LanceTableOperations.dropTable`: `super.dropTable` succeeds,
`dropLanceDataset` fails and is wrapped), both
`tableFormatCache.invalidate(ident)` and the unprovision below are skipped: the
table is gone, the location leaks, and unlike the provider-failure path there
is no WARN naming it. Suggest `try { dropped = ...; } finally {
tableFormatCache.invalidate(ident); }` and a WARN with the location in a
`catch` before rethrowing (not an unprovision — the reasons you give for
limitation (2) apply here too).
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/GenericCatalogOperations.java:
##########
@@ -273,67 +322,138 @@ public Table alterTable(NameIdentifier ident,
TableChange... changes)
@Override
public boolean purgeTable(NameIdentifier ident) {
- boolean purged = tableOps(ident).purgeTable(ident);
- tableFormatCache.invalidate(ident);
- return purged;
+ return dropOrPurgeTable(ident, true /* purge */);
}
@Override
public boolean dropTable(NameIdentifier ident) throws
UnsupportedOperationException {
- boolean dropped = tableOps(ident).dropTable(ident);
- tableFormatCache.invalidate(ident);
- return dropped;
+ return dropOrPurgeTable(ident, false /* purge */);
}
- private String calculateTableLocation(
- Schema schema, NameIdentifier tableIdent, Map<String, String>
tableProperties) {
- String tableLocation =
- (String)
- propertiesMetadata
- .tablePropertiesMetadata()
- .getOrDefault(tableProperties, Table.PROPERTY_LOCATION);
- if (StringUtils.isNotBlank(tableLocation)) {
- return ensureTrailingSlash(tableLocation);
- }
+ /**
+ * Returns the cache mapping a table to its format, so that tests can assert
it is kept in step
+ * with the tables that exist.
+ *
+ * @return the table format cache
+ */
+ @VisibleForTesting
+ Cache<NameIdentifier, String> tableFormatCache() {
+ return tableFormatCache;
+ }
- String schemaLocation =
- schema.properties() == null ? null :
schema.properties().get(Schema.PROPERTY_LOCATION);
+ /**
+ * Drops or purges a table, and hands its location back to the {@link
TableLocationProvider}
+ * afterwards.
+ *
+ * <p>The table properties are read before the removal, because they carry
the location the
+ * provider has to hand back, and the unprovisioning itself happens after
the removal so that a
+ * provider never reclaims the storage of a table that is still there. A
provider failing to
+ * unprovision is logged at WARN rather than propagated: the table is
already gone at that point,
+ * so failing the request would report a drop that did in fact happen as
unsuccessful and invite a
+ * retry that cannot undo anything.
+ *
+ * @param ident the identifier of the table to drop
+ * @param purge whether to purge the table instead of dropping it
+ * @return true if the table was dropped, false if it did not exist
+ */
+ private boolean dropOrPurgeTable(NameIdentifier ident, boolean purge) {
+ return dropOrPurgeTable(
+ ident, purge,
loadSchema(NameIdentifier.of(ident.namespace().levels())));
+ }
- // If we do not set location in table properties, and schema location is
set, use schema
- // location as the base path.
- if (StringUtils.isNotBlank(schemaLocation)) {
- return ensureTrailingSlash(schemaLocation) + tableIdent.name() + SLASH;
+ /**
+ * Drops or purges a table, resolving its parent schema through the given
supplier.
+ *
+ * <p>The schema is passed in rather than loaded here so that a cascading
schema drop, where every
+ * table shares one parent, loads it once instead of once per table. It
stays eager: the context
+ * is built in full before the table is removed, so that a store read
failing fails the request
+ * while the table is still there, rather than from inside a callback where
it could only be
+ * reported as a provider failure it is not.
+ *
+ * @param ident the identifier of the table to drop
+ * @param purge whether to purge the table instead of dropping it
+ * @param schema the table's parent schema
+ * @return true if the table was dropped, false if it did not exist
+ */
+ private boolean dropOrPurgeTable(NameIdentifier ident, boolean purge, Schema
schema) {
+ Map<String, String> tableProperties;
+ try {
+ tableProperties = store.get(ident, TABLE,
TableEntity.class).properties();
+ } catch (NoSuchEntityException e) {
+ return false;
+ } catch (IOException e) {
+ throw new RuntimeException(
+ String.format("Failed to load table %s before dropping it", ident),
e);
}
- // If the schema location is not set, use catalog lakehouse dir as the
base path. Or else, throw
- // an exception.
- if (catalogLocation.isEmpty()) {
- throw new IllegalArgumentException(
- "'location' property is neither set in table properties "
- + "nor in schema properties, and no location is set in catalog
properties either. "
- + "Please set the 'location' in either of them to create the
table "
- + tableIdent);
+ // Built entirely before the drop, so that a store read failing here fails
the request while
+ // the table is still there, rather than after it is gone where it could
only be reported as a
+ // provider failure it is not.
+ TableLocationContext context =
+ TableLocationContext.builder()
+ .withTableIdentifier(ident)
+ .withTableProperties(tableProperties)
+ .withSchema(schema)
+ .build();
+
+ // The properties just read are handed on rather than left to be read
again: resolving the
+ // table format is a second store read for the very same entity whenever
the format cache is
+ // cold, which for a drop it usually is.
+ ManagedTableOperations tableOps = tableOps(ident, tableProperties);
+ boolean dropped = purge ? tableOps.purgeTable(ident) :
tableOps.dropTable(ident);
+ tableFormatCache.invalidate(ident);
+
+ if (dropped && !context.isExternal()) {
Review Comment:
`purge` of an **external** Lance table does delete the dataset
(`LanceTableOperations.purgeTable`: `external && purged → dropLanceDataset`),
so for an external table that was created *without* a location — which, as the
description says, still gets a provider-allocated path — a purge deletes the
data and then never hands the allocation back. The "the formats leave an
external dataset in place" justification holds for drop, not for purge. Minimal
fix: don't skip on `purge`, or add `isPurge()` to `TableLocationContext` and
let the provider decide.
##########
docs/lakehouse-generic-catalog.md:
##########
@@ -48,13 +48,200 @@ For detailed information on available operations, see
[Manage Relational Metadat
|-----------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-------------------------|----------|
| `provider` | Catalog provider type
| `lakehouse-generic` | Yes |
| `location` | Root storage path for all schemas and tables
| `s3://bucket/lakehouse` | No |
+| `table-location-provider` | Name of the [table location
provider](#pluggable-table-location-provider) that provisions and unprovisions
the locations of this catalog's tables. Defaults to `default`, which resolves
the location from the table, schema and catalog `location` properties as
described below. Immutable once the catalog is created.
| `default` | No |
Review Comment:
Nit: the new row isn't pipe-aligned with the rest of the table (description
cell overflows the column; separator row not widened).
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/GenericCatalogOperations.java:
##########
@@ -129,22 +132,35 @@ protected EntityStore store() {
public void initialize(
Map<String, String> conf, CatalogInfo info, HasPropertyMetadata
propertiesMetadata)
throws RuntimeException {
- this.catalogProperties = conf == null ? Map.of() : Maps.newHashMap(conf);
- String location =
+ // A defensive copy that tolerates null values, for the same reason as
+ // TableLocationContext.Builder#withTableProperties: nothing upstream
rejects a catalog
+ // property whose value is null, and ImmutableMap.copyOf would turn one
into a
+ // NullPointerException. Here it would fail the creation of the whole
catalog rather than a
+ // single request.
+ this.catalogProperties =
+ conf == null ? Map.of() :
Collections.unmodifiableMap(Maps.newHashMap(conf));
+
+ String providerName =
(String)
propertiesMetadata
.catalogPropertiesMetadata()
- .getOrDefault(conf, Catalog.PROPERTY_LOCATION);
- this.catalogLocation =
- StringUtils.isNotBlank(location)
- ? Optional.of(location).map(this::ensureTrailingSlash)
- : Optional.empty();
- this.propertiesMetadata = propertiesMetadata;
+ .getOrDefault(conf,
GenericCatalogPropertiesMetadata.TABLE_LOCATION_PROVIDER);
Review Comment:
Two things about the null tolerance:
1. `PropertiesMetadata.getOrDefault` returns `decode(null)` when the key is
*present* with a null value, so `table-location-provider` present-but-null
reaches `TableLocationProviderFactory.create` as a blank name and fails
initialization with "must not be blank" instead of falling back to `default`,
which contradicts the intent of the copy above.
`StringUtils.defaultIfBlank(providerName, DefaultTableLocationProvider.NAME)`
(or an explicit, clearer rejection) would close it.
2. The same unmodifiable map is handed to
`LanceTableOperations.setCatalogProperties`, which does
`ImmutableMap.copyOf(...)` and NPEs on a null value — so with the real Lance
format every `createTable`/`loadTable`/`dropTable` on such a catalog still
fails in `configureTableOps`. Pre-existing, but the new comment and
`testACatalogPropertyWithANullValueDoesNotFailInitialization` only hold for
`FakeTableDelegator`. Either finish the tolerance (fix that copy too) or reject
null values at validation time and drop the comment/test; the half-way state is
what misleads.
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/TableLocationProviderFactory.java:
##########
@@ -0,0 +1,274 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.catalog.lakehouse.generic;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.lang.ref.WeakReference;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Optional;
+import java.util.ServiceConfigurationError;
+import java.util.ServiceLoader;
+import java.util.Set;
+import java.util.WeakHashMap;
+import java.util.stream.Collectors;
+import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A factory that discovers {@link TableLocationProvider}s through {@link
ServiceLoader}.
+ *
+ * <p>Discovery is done once per class loader and remembered, because it is
the expensive half:
+ * selecting by name requires every registered provider to be instantiated so
that {@link
+ * TableLocationProvider#name()} can be called on it, and {@code name()} is an
instance method, so
+ * there is no way to learn the names without doing that at least once. What
the cache removes is
+ * repeating it for every catalog; it cannot remove the first pass. Creating a
catalog after the
+ * first then instantiates only the provider it selected.
+ *
+ * <p>The remembered index holds classes, and a class keeps its class loader
alive, so the entries
+ * are weak on both sides: the map is keyed weakly by class loader and holds
each class through a
+ * {@link WeakReference}. A strong value would pin the loader of a dropped
catalog through the very
+ * map meant to speed the next one up, which is the leak {@code [#12986]}
removed elsewhere. A
+ * collected entry simply causes the next lookup to scan again.
+ */
+public class TableLocationProviderFactory {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(TableLocationProviderFactory.class);
+
+ /**
+ * Provider classes by lower-cased name, per class loader. Weak on both
sides; see the class
+ * javadoc. Guarded by synchronization on the map itself rather than by a
concurrent map, because
+ * {@link WeakHashMap} is not thread-safe and the map is touched once per
catalog creation.
+ */
+ private static final Map<ClassLoader, Index> INDEXES =
Review Comment:
Non-blocking, but I think this cache is more machinery than the deployment
topology warrants. `org.apache.gravitino.catalog.lakehouse.*` matches
`IsolatedClassLoader.isCatalogClass`, so `TableLocationProviderFactory` — and
this static map — is loaded once *per isolated loader*, and the `cl` used as
the key (the TCCL during `initialize`) is that same loader. The map therefore
never holds more than one entry, whose key is the loader that owns the map, and
the `WeakReference<Class>` values point at classes from the same loader as the
map itself; the weak references have nothing to protect against. What remains
is skipping N trivial constructions when
`gravitino.catalog.classloader.sharing.enabled` puts a second catalog on the
same loader, and the contract already requires those constructors to be trivial.
`LakehouseTableDelegatorFactory` in the same module simply rescans per
catalog; matching it removes `Index`, `INDEXES`, `invalidateCache()`, two tests
and a javadoc that describes a leak scenario this codebase can't produce.
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/GenericCatalogOperations.java:
##########
@@ -192,9 +223,14 @@ public boolean dropSchema(NameIdentifier ident, boolean
cascade) throws NonEmpty
"Schema %s is not empty, cannot drop it without cascade", ident);
}
- // Drop all tables under the schema first if cascade is true.
+ // Drop all tables under the schema first if cascade is true. This goes
through the same path
+ // as the catalog level dropTable, so that the location of each table is
unprovisioned and its
+ // cached format invalidated. The schema is resolved once for the whole
cascade rather than
+ // once per table: every table here has the same parent, and the provider
may not ask for it at
+ // all.
+ Schema cascadedSchema = loadSchema(ident);
Review Comment:
This runs even when `tableIdents` is empty (every non-cascading drop of an
empty schema now pays a schema read). Move it inside `if (tableIdents.length >
0)`.
--
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]
