shaoyu-li commented on code in PR #13058:
URL: https://github.com/apache/gravitino/pull/13058#discussion_r3985825610
##########
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:
Done in 29a4ea9. `unprovisionTableLocation` is not called for an external
table, on the single drop
path and on a cascade; `testDroppingExternalTableSkipsUnprovision` and
`testDroppingExternalTableSkipsUnprovisionInACascade` pin both.
One thing that came out of implementing it, worth flagging because it is a
real hazard rather than
a detail. `external` is stored as the string the caller sent and decoded by
the property metadata
with `Boolean::valueOf`, which accepts `external=yes` without complaint and
means false by it. Both
formats read it the same way -- `Boolean.parseBoolean`, or an
`equalsIgnoreCase("true")`. The first
version of `isExternal()` used `BooleanUtils.toBoolean`, which is true for
`yes`, `on`, `y` and `t`.
That disagreement is the bad direction: dropping a Lance table created with
`external=yes` would
have the format read false and delete the dataset, while the catalog read
true and skipped handing
the location back -- the data gone and the allocation leaked in one drop, on
a property value the
server accepts. Fixed in be4c82f to `Boolean.parseBoolean`, with a test over
every value the old
reading accepted.
--
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]
