roryqi commented on code in PR #11364:
URL: https://github.com/apache/gravitino/pull/11364#discussion_r3341588517
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergTableOperationExecutor.java:
##########
@@ -110,15 +121,38 @@ public LoadTableResponse updateTable(
@Override
public void dropTable(
IcebergRequestContext context, TableIdentifier tableIdentifier, boolean
purgeRequested) {
- if (purgeRequested) {
- icebergCatalogWrapperManager
- .getCatalogWrapper(context.catalogName())
- .purgeTable(tableIdentifier);
- } else {
- icebergCatalogWrapperManager
- .getCatalogWrapper(context.catalogName())
- .dropTable(tableIdentifier);
+ IcebergCatalogWrapper wrapper =
+ icebergCatalogWrapperManager.getCatalogWrapper(context.catalogName());
+ if (!purgeRequested) {
+ wrapper.dropTable(tableIdentifier);
+ return;
+ }
+
+ // Async cleanup is opt-in per request and only wired in auxiliary mode;
otherwise purge inline.
+ if (!context.asyncPurge()) {
+ wrapper.purgeTable(tableIdentifier);
+ return;
}
+
+ cleanupManager.ifPresentOrElse(
+ manager -> {
+ // Read the metadata location before dropping the catalog entry,
then enqueue the job. The
+ // job deletes only files reachable from this old metadata, so a
table recreated at the
+ // same name (with fresh metadata) is never touched.
+ TableMetadata metadata = wrapper.loadTableMetadata(tableIdentifier);
+ wrapper.dropTable(tableIdentifier);
+ manager.addJob(
+ new IcebergCleanupJob(
Review Comment:
This ordering is intentional. The cleanup job deletes everything reachable
from the metadata location, which until the catalog entry is dropped still
points at the *live* table — so enqueuing before the drop could aim a deletion
job at a live table's files. Dropping first guarantees a job only ever exists
for a table that is already gone (design §5.3).
If `addJob` fails, the request returns 5xx and the dropped table's files are
leaked — but that is the same leak the design already tolerates for terminally
FAILED jobs (§5.6), not a new failure mode. Compensating here (re-creating the
catalog entry, or inline-deleting) would add real complexity and new failure
paths for a rare transient backend error, so we keep it as-is.
##########
iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ops/IcebergCatalogWrapper.java:
##########
@@ -240,6 +242,36 @@ public void purgeTable(TableIdentifier tableIdentifier) {
CatalogHandlers.purgeTable(getCatalog(), tableIdentifier);
}
+ /**
+ * Loads current {@link TableMetadata}, bypassing the response cache. Used
by the async cleanup
+ * path to snapshot the metadata location before dropping the catalog entry.
+ *
+ * @param tableIdentifier the table
+ * @return its metadata
+ */
+ public TableMetadata loadTableMetadata(TableIdentifier tableIdentifier) {
+ return ((BaseTable)
getCatalog().loadTable(tableIdentifier)).operations().current();
+ }
+
+ /**
+ * Returns the FileIO implementation configured for this catalog.
+ *
+ * @return the {@code io-impl} class, or the Iceberg default when unset
+ */
+ public String fileIOImpl() {
+ String impl = icebergConfig.get(IcebergConfig.IO_IMPL);
+ return StringUtils.isNotBlank(impl) ? impl :
ResolvingFileIO.class.getName();
+ }
+
+ /**
+ * Returns catalog properties used to reconstruct FileIO in a cleanup worker.
+ *
+ * @return catalog properties snapshotted at enqueue time
+ */
+ public Map<String, String> fileIOProperties() {
+ return getIcebergConfig().getIcebergCatalogProperties();
+ }
Review Comment:
By design. This is the same property map the wrapper uses to build the
catalog and its FileIO (`IcebergCatalogWrapper` ctor), so reusing it is what
guarantees the worker reconstructs an identical FileIO; a hand-picked subset
would risk dropping a key some FileIO impl needs (S3/GCS/ADLS each read
different keys). Credentials in `file_io_props` are explicitly acknowledged and
accepted in the design (§5.11 — relies on the existing Gravitino DB encryption
/ access controls). The extra non-FileIO keys are minor row bloat and not worth
the fragility of precise filtering.
--
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]
