czy006 commented on code in PR #4293:
URL: https://github.com/apache/amoro/pull/4293#discussion_r3822984563
##########
amoro-format-iceberg/src/main/java/org/apache/amoro/formats/iceberg/utils/IcebergTableUtil.java:
##########
@@ -193,4 +202,95 @@ public static Set<String> getAllManifestFiles(Table table)
{
return allManifestFiles;
}
+
+ private static boolean isMetadataJson(String name) {
+ return name.endsWith(".metadata.json") ||
name.endsWith(".metadata.json.gz");
+ }
+
+ /**
+ * Returns {@code true} if another Iceberg table appears to share the same
metadata location as
+ * the given table.
+ *
+ * <p>Lists the table's metadata directory via its own (storage-agnostic)
{@link FileIO} and, for
+ * every {@code metadata.json} not in this table's own history (current +
{@code
+ * previousFiles()}), reads its {@code table-uuid}:
+ *
+ * <ul>
+ * <li>same uuid → older version of this table, ignored;
+ * <li>different uuid → another table shares the location, returns
{@code true};
+ * <li>uuid missing/unreadable (legacy, corrupt, or compressed) →
treated as a conflict,
+ * returns {@code true} (fail-safe).
+ * </ul>
+ *
+ * <p>Requires {@link SupportsPrefixOperations} for FileIO; otherwise a
{@link
+ * ValidationException} is thrown. The caller may choose to skip the check
when the storage
+ * backend is known to be used by a single table. On the no-conflict path
only a single listing is
+ * done and no metadata file is read.
+ *
+ * @param table the table whose location is about to be cleaned
+ * @throws ValidationException if the table's {@code FileIO} does not
support prefix operations
+ */
+ public static boolean hasOtherTableInLocation(Table table) {
+ TableOperations ops = ((HasTableOperations) table).operations();
+ TableMetadata current = ops.current();
+
+ String myUuid = current.uuid();
+ Set<String> myMetadataFiles = Sets.newHashSet();
+ myMetadataFiles.add(new Path(current.metadataFileLocation()).getName());
+ for (TableMetadata.MetadataLogEntry entry : current.previousFiles()) {
+ myMetadataFiles.add(new Path(entry.file()).getName());
+ }
+
+ Path metadataDir = new Path(current.metadataFileLocation()).getParent();
+
+ AuthenticatedFileIO io = (AuthenticatedFileIO) table.io();
Review Comment:
**Required:** This unconditional cast makes the new test fail with
`ClassCastException` when `table.io()` is a regular Iceberg `FileIO`; the
expected `ValidationException` is never reached. I reproduced this in
`TestIcebergOrphanFileLocationConflict.testHasOtherTableInLocationThrowsWhenFileIoLacksPrefixSupport`.
Please handle plain `FileIO` instances with `instanceof` and use
`AuthenticatedFileIO.asPrefixFileIO()` for authenticated wrappers instead of
casting blindly.
##########
amoro-format-iceberg/src/main/java/org/apache/amoro/formats/iceberg/maintainer/IcebergTableMaintainer.java:
##########
@@ -144,22 +158,84 @@ public Map<String, String> cleanOrphanFiles() {
return Maps.newHashMap();
}
+ LocationConflictCheckResult checkResult =
checkLocationConflict(tableConfiguration);
+
+ switch (checkResult) {
+ case CONFLICT_DETECTED:
+ metrics.recordFailure(CleanFailureReason.LOCATION_CONFLICT);
+ return Maps.newHashMap();
+ case CONFLICT_CHECK_FAILED:
+
metrics.recordFailure(CleanFailureReason.LOCATION_CONFLICT_CHECK_FAILED);
+ return Maps.newHashMap();
+ default:
+ break;
+ }
+
long keepTime = tableConfiguration.getOrphanExistingMinutes() * 60 * 1000;
- int dataDeleted = cleanContentFiles(System.currentTimeMillis() - keepTime,
metrics);
+ int dataDeleted;
+ int metadataDeleted;
+ try {
+ dataDeleted = cleanContentFiles(System.currentTimeMillis() - keepTime,
metrics);
+
+ // refresh
+ table.refresh();
- // refresh
- table.refresh();
+ // clear metadata files
+ metadataDeleted = cleanMetadata(System.currentTimeMillis() - keepTime,
metrics);
- // clear metadata files
- int metadataDeleted = cleanMetadata(System.currentTimeMillis() - keepTime,
metrics);
+ // cleanup completed — reset status to SUCCESS
Review Comment:
**Required:** In a mixed table, both the change and base maintainers share
the same metrics instance and execute this success update. If the change table
records `LOCATION_CONFLICT` and the base table then succeeds, this line
overwrites the final status to `SUCCESS`, hiding the skipped cleanup. Please
aggregate child outcomes in `MixedTableMaintainer` and record one final status
after both children complete.
##########
amoro-common/src/main/java/org/apache/amoro/maintainer/MaintainerMetrics.java:
##########
@@ -40,6 +40,36 @@ public interface MaintainerMetrics {
*/
void recordOrphanMetadataFilesCleaned(int expected, int cleaned);
+ /** Record a successful orphan-file-cleaning run. */
+ void recordSuccess();
Review Comment:
**Consider:** Adding new abstract methods to this public interface can break
external implementations at source or runtime (`AbstractMethodError`). Consider
default no-op methods or a separate optional interface for cleanup-status
metrics.
##########
amoro-format-iceberg/src/main/java/org/apache/amoro/formats/iceberg/utils/IcebergTableUtil.java:
##########
@@ -193,4 +202,95 @@ public static Set<String> getAllManifestFiles(Table table)
{
return allManifestFiles;
}
+
+ private static boolean isMetadataJson(String name) {
+ return name.endsWith(".metadata.json") ||
name.endsWith(".metadata.json.gz");
+ }
+
+ /**
+ * Returns {@code true} if another Iceberg table appears to share the same
metadata location as
+ * the given table.
+ *
+ * <p>Lists the table's metadata directory via its own (storage-agnostic)
{@link FileIO} and, for
+ * every {@code metadata.json} not in this table's own history (current +
{@code
+ * previousFiles()}), reads its {@code table-uuid}:
+ *
+ * <ul>
+ * <li>same uuid → older version of this table, ignored;
+ * <li>different uuid → another table shares the location, returns
{@code true};
+ * <li>uuid missing/unreadable (legacy, corrupt, or compressed) →
treated as a conflict,
+ * returns {@code true} (fail-safe).
+ * </ul>
+ *
+ * <p>Requires {@link SupportsPrefixOperations} for FileIO; otherwise a
{@link
+ * ValidationException} is thrown. The caller may choose to skip the check
when the storage
+ * backend is known to be used by a single table. On the no-conflict path
only a single listing is
+ * done and no metadata file is read.
+ *
+ * @param table the table whose location is about to be cleaned
+ * @throws ValidationException if the table's {@code FileIO} does not
support prefix operations
+ */
+ public static boolean hasOtherTableInLocation(Table table) {
+ TableOperations ops = ((HasTableOperations) table).operations();
+ TableMetadata current = ops.current();
+
+ String myUuid = current.uuid();
+ Set<String> myMetadataFiles = Sets.newHashSet();
+ myMetadataFiles.add(new Path(current.metadataFileLocation()).getName());
+ for (TableMetadata.MetadataLogEntry entry : current.previousFiles()) {
+ myMetadataFiles.add(new Path(entry.file()).getName());
+ }
+
+ Path metadataDir = new Path(current.metadataFileLocation()).getParent();
Review Comment:
**Critical:** The guard derives the scan directory from
`current.metadataFileLocation()`, while
`IcebergTableMaintainer.clearInternalTableMetadata()` deletes from
`table.location()/metadata`. With a custom metadata location, or with two
tables sharing the data location but using different metadata directories, this
check can miss the other table and the cleanup can still delete its files.
Please make the guard use the same canonical metadata location as the cleanup
path and add a regression test for `write.metadata.location`.
##########
amoro-ams/src/test/java/org/apache/amoro/server/table/TestTableOrphanFilesCleaningMetrics.java:
##########
@@ -0,0 +1,110 @@
+/*
+ * 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.amoro.server.table;
+
+import org.apache.amoro.ServerTableIdentifier;
+import org.apache.amoro.TableFormat;
+import org.apache.amoro.metrics.MetricRegistry;
+import org.apache.amoro.maintainer.MaintainerMetrics.CleanFailureReason;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
Review Comment:
**Required:** This import order currently fails the project's Spotless
check. The normal Maven build stops before running tests with a format
violation in this file. Please run Spotless and commit the formatted import
order.
--
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]
