Copilot commented on code in PR #11364:
URL: https://github.com/apache/gravitino/pull/11364#discussion_r3341279377
##########
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:
In the async purge path, the table is dropped before the cleanup job is
persisted. If `addJob` throws (DB/session error), the request will fail after
the catalog entry is already gone, leaving (1) orphaned table files and (2) no
tombstone row to block immediate name reuse. This needs explicit failure
handling so an enqueue failure cannot silently break purge guarantees.
##########
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:
`fileIOProperties()` returns `IcebergConfig.getIcebergCatalogProperties()`,
which (per `IcebergConfig#getIcebergCatalogProperties`) includes *all* Iceberg
REST server config entries in addition to catalog properties. These properties
are serialized into `iceberg_cleanup_job.file_io_props`, so this can
unnecessarily persist unrelated (and potentially sensitive) settings (e.g.,
JDBC passwords, object-store secrets) and bloat the row. Consider
filtering/snapshotting only the minimal set of keys required to reconstruct
FileIO for deletion.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java:
##########
@@ -198,6 +220,7 @@ public void serviceInit(Map<String, String> properties,
boolean auxMode) {
@Override
public void serviceStart() {
icebergMetricsManager.start();
+ cleanupManager.ifPresent(IcebergCleanupManager::start);
if (server != null) {
Review Comment:
If `server.start()` throws, `cleanupManager.start()` has already been called
and will keep background threads running even though the service failed to
start. The catch block should stop the cleanup manager (and ideally other
started components) before rethrowing to avoid leaking resources on startup
failure.
--
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]
