yuqi1129 commented on code in PR #13058:
URL: https://github.com/apache/gravitino/pull/13058#discussion_r3978554341
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/GenericCatalogOperations.java:
##########
@@ -233,7 +250,16 @@ public Table createTable(
Index[] indexes)
throws NoSuchSchemaException, TableAlreadyExistsException {
Schema schema = loadSchema(NameIdentifier.of(ident.namespace().levels()));
- String tableLocation = calculateTableLocation(schema, ident, properties);
+ String tableLocation =
+ validateProvisionedLocation(
+ tableLocationProvider.provisionTableLocation(
Review Comment:
**Blocking (correctness): external and registered tables.** This calls the
provider unconditionally, and the returned value always wins --
`newProperties.put(Table.PROPERTY_LOCATION, tableLocation)` a few lines down
overwrites whatever the user passed.
For an external table the user-supplied `location` is not a hint, it is the
whole point: `GenericTablePropertiesMetadata.java:43` documents it as required
for external tables, and `LanceTableOperations.createTable` supports
registering an existing dataset. With a custom allocator provider selected,
registering an existing dataset would silently point the table somewhere else.
Please skip provisioning when `Table.PROPERTY_EXTERNAL` is true (and expose
`isExternal()` on `TableLocationContext` so a provider can see it too).
**Blocking (correctness): a failed creation leaks the allocation.**
Provisioning happens *before* the format is validated (`format != null`),
before `tableOpsCache.get(format)` is looked up, and before
`tableOps.createTable(...)` runs. A missing format, an unsupported format, a
`TableAlreadyExistsException` or any failure inside the delegator all leave a
location allocated from the external service with nothing left to reclaim it --
`unprovisionTableLocation` never fires, because the table does not exist to be
dropped.
Unlike the drop path, this one is cheap to get right and the asymmetry
argument in the PR description does not apply: the table demonstrably was not
created, so reclaiming is safe. Suggest moving the format validation and the
`tableOpsCache` lookup above the provisioning, and wrapping
`tableOps.createTable(...)` in a try/catch that unprovisions (swallowing any
secondary failure with a WARN) before rethrowing.
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/GenericCatalogOperations.java:
##########
@@ -273,57 +299,80 @@ 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);
+ /**
+ * 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) {
+ 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);
}
- String schemaLocation =
- schema.properties() == null ? null :
schema.properties().get(Schema.PROPERTY_LOCATION);
-
- // 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;
- }
+ // 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(loadSchema(NameIdentifier.of(ident.namespace().levels())))
+ .build();
+
+ boolean dropped = purge ? tableOps(ident).purgeTable(ident) :
tableOps(ident).dropTable(ident);
+ tableFormatCache.invalidate(ident);
- // 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);
+ if (dropped) {
+ try {
+ tableLocationProvider.unprovisionTableLocation(context);
Review Comment:
**Blocking (correctness): this fires for external tables too.**
`LanceTableOperations.dropTable` (lines 354-379) and `purgeTable` (317-347)
both read `Table.PROPERTY_EXTERNAL` and deliberately do *not* delete the
dataset when it is set -- the storage belongs to the user, Gravitino only
registered it. This call has no such guard, so a provider that genuinely
reclaims storage will release a location Gravitino never allocated.
By the PR's own standard ("Leaked storage is recoverable; a premature
reclaim is data loss") this is the failure mode worth defending against.
Suggest mirroring the Lance check and skipping the unprovision when the dropped
table was external.
Everything else about this block looks right to me: reading the properties
before the removal, unprovisioning after it, and downgrading a provider failure
to a WARN are all the correct calls, and the reasoning in the javadoc is sound.
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/GenericCatalogOperations.java:
##########
@@ -273,57 +299,80 @@ 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);
+ /**
+ * 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) {
+ Map<String, String> tableProperties;
+ try {
+ tableProperties = store.get(ident, TABLE,
TableEntity.class).properties();
+ } catch (NoSuchEntityException e) {
Review Comment:
**Undeclared behaviour change.** Before this PR,
`dropTable(nonExistentTable)` reached `tableOps(ident)`, whose `store.get`
threw `NoSuchEntityException`, which the catch block at the bottom of
`tableOps` converted into `NoSuchTableException`. Now it returns `false` here.
I actually prefer the new behaviour -- returning `false` is what
`TableCatalog#dropTable` documents -- but it is visible through REST (an error
response becomes `dropped: false`), so "No behavioural change for existing
catalogs" in the PR description is not accurate. No existing unit test or IT
covers it, so CI will not flag it either. Could you state it in the description
and add a test pinning the chosen semantics?
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/GenericCatalogOperations.java:
##########
@@ -273,57 +299,80 @@ 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);
+ /**
+ * 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) {
+ 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);
}
- String schemaLocation =
- schema.properties() == null ? null :
schema.properties().get(Schema.PROPERTY_LOCATION);
-
- // 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;
- }
+ // 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(loadSchema(NameIdentifier.of(ident.namespace().levels())))
Review Comment:
**Efficiency: the same rows are read up to four times per drop.**
Within a single `dropOrPurgeTable`:
- line 328 reads the `TableEntity`;
- `tableOps(ident)` on line 346 reads *the same* `TableEntity` again
whenever `tableFormatCache` misses (it needs `PROPERTY_TABLE_FORMAT`);
- `LanceTableOperations.dropTable` then calls `super.loadTable(ident)`,
reading it a third time;
- and this line adds a schema read on top.
For `DefaultTableLocationProvider` none of it is needed --
`unprovisionTableLocation` is an empty body -- so the default deployment pays
two extra store reads per drop for nothing. In the cascade path it is worse:
every table reloads the *same* schema, so dropping a schema with N tables does
N identical `loadSchema` calls.
Since you deliberately (and I think correctly) kept
`unprovisionTableLocation` abstract, a capability flag preserves that intent
while letting the common path skip the work:
```java
default boolean requiresUnprovision() {
return false;
}
```
`DefaultTableLocationProvider` leaves it alone, a provider that reclaims
real storage overrides it to `true`, and `dropOrPurgeTable` builds the context
only when it is set. Hoisting the schema out of the cascade loop (see my
comment on `dropSchema`) would take care of the rest; a `Supplier<Schema>`
inside `TableLocationContext` would be an alternative.
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/GenericCatalogOperations.java:
##########
@@ -192,9 +207,11 @@ 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 catalog
+ // level dropTable, so that the location of each table is unprovisioned
and its cached format
+ // invalidated.
for (NameIdentifier tableIdent : tableIdents) {
- tableOps(tableIdent).dropTable(tableIdent);
+ dropTable(tableIdent);
Review Comment:
Routing the cascade through the catalog-level `dropTable` is the right call,
and the `tableFormatCache` invalidation it fixes was a real bug.
One efficiency note: each iteration now re-enters `dropOrPurgeTable`, which
loads the parent schema again -- N identical `loadSchema` calls for N tables,
even though `dropSchema` could load it once. Passing the already-loaded schema
down (or resolving it lazily in `TableLocationContext`) would avoid that.
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/GenericCatalogOperations.java:
##########
@@ -367,4 +416,31 @@ private ManagedTableOperations
configureTableOps(ManagedTableOperations ops) {
return ops;
}
+
+ /**
+ * Validates the location returned by a {@link TableLocationProvider} before
it is stored in the
+ * table properties, so that a misbehaving provider fails the table creation
instead of silently
+ * producing a broken location.
+ *
+ * <p>Only blankness is checked. The shape of the path belongs to the
provider: nothing downstream
+ * appends to the location, and the value is stored verbatim so that a
provider unprovisioning it
+ * later sees exactly the string it returned.
+ *
+ * @param location the location returned by the provider
+ * @param providerName the name of the provider that returned it
+ * @param tableIdent the identifier of the table being created
+ * @return the validated location
+ * @throws IllegalArgumentException if the location is blank
+ */
+ @VisibleForTesting
+ static String validateProvisionedLocation(
Review Comment:
Minor, style only:
- `AGENTS.md` asks for `static` members before instance ones and `private`
methods last; this `static` method sits between private instance methods, as
does the `@VisibleForTesting tableFormatCache()` accessor.
- The body is a single `Preconditions.checkArgument`. Inlining it at the
call site in `createTable` would drop a three-parameter method plus ~20 lines
of javadoc, and the `@VisibleForTesting` hook is not needed --
`TestGenericCatalogOperations` already covers the blank-location rejection
through `createTable`.
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/TableLocationProviderFactory.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.base.Preconditions;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.ServiceLoader;
+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}. */
+public class TableLocationProviderFactory {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(TableLocationProviderFactory.class);
+
+ private TableLocationProviderFactory() {}
+
+ /**
+ * Creates and initializes the {@link TableLocationProvider} registered
under the given name.
+ *
+ * <p>A new instance is returned on every call, so the caller owns its
lifecycle and is
+ * responsible for closing it.
+ *
+ * @param name the provider name to look up, matched case-insensitively
against {@link
+ * TableLocationProvider#name()}
+ * @param catalogProperties the properties of the catalog the provider
belongs to
+ * @return the initialized provider
+ * @throws IllegalArgumentException if no provider, or more than one
provider, is registered under
+ * the given name
+ */
+ public static TableLocationProvider create(String name, Map<String, String>
catalogProperties) {
+ Preconditions.checkArgument(
+ StringUtils.isNotBlank(name), "Table location provider name must not
be blank");
+
+ ClassLoader cl =
+ Optional.ofNullable(Thread.currentThread().getContextClassLoader())
+ .orElse(TableLocationProvider.class.getClassLoader());
+ ServiceLoader<TableLocationProvider> loader =
+ ServiceLoader.load(TableLocationProvider.class, cl);
+
+ List<TableLocationProvider> providers =
+ loader.stream()
Review Comment:
`ServiceLoader.Provider::get` instantiates **every** provider on the
classpath before the filter runs, and the ones that lose are neither
initialized nor closed. The docs call the constraint out, so I am fine with the
semantics -- but this whole scan runs on every catalog initialization.
The sibling SPI in this module resolves it once and caches:
`LakehouseTableDelegatorFactory` keeps a `private static ImmutableMap` built
under a `synchronized` initializer. Matching that here -- cache the discovered
`name -> Class` mapping once, then reflectively instantiate only the selected
one per catalog -- would keep the two factories consistent, avoid instantiating
unrelated third-party providers, and surface a duplicate `name()` at discovery
time rather than on the unlucky catalog.
--
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]
