shaoyu-li commented on code in PR #13058:
URL: https://github.com/apache/gravitino/pull/13058#discussion_r4008543182
##########
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:
Confirmed and fixed in 8ffd3c2, including your suggested shape. The drop is
wrapped: the format
cache is invalidated in a `finally`, and a `catch` logs a WARN naming the
table, the location and
the provider before rethrowing.
Your reading of the consequence is the part I had missed. A stale cache
entry here is not merely
untidy -- the table is gone, so the next table created under the same name
reads a format decided
by its predecessor.
Not an unprovision, for exactly the reason you give: the format may equally
have failed before
touching storage, so reclaiming the path is the unrecoverable direction.
Pinned by
`testAFormatFailingAfterRemovingMetadataStillInvalidatesTheFormatCache`,
which also asserts no
unprovision happens; reverting the `finally` makes it fail.
--
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]
