j1wonpark commented on code in PR #4293:
URL: https://github.com/apache/amoro/pull/4293#discussion_r3944023364
##########
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:
Review Comment:
question: This also fires for a table that was dropped without purge and
re-created at the same location — the old uuid's `metadata.json` files remain,
so cleanup for such tables stops permanently with only a WARN log. Is that
acceptable as a default, or should the release notes at least call it out with
the opt-out property?
##########
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();
+ if (!io.supportPrefixOperations()) {
+ String msg =
+ String.format(
+ "Cannot detect location conflicts: the table's FileIO (%s) does
not support prefix "
+ + "operations, which are required to inspect the metadata
directory '%s'.",
+ io.getClass().getName(), metadataDir);
+ throw new ValidationException(msg);
+ }
+
+ String prefix = metadataDir.toString();
+ if (!prefix.endsWith("/")) {
+ prefix = prefix + "/";
+ }
+
+ SupportsPrefixOperations prefixIo = (SupportsPrefixOperations) io;
+ for (FileInfo info : prefixIo.listPrefix(prefix)) {
+ String name = new Path(info.location()).getName();
+ if (!isMetadataJson(name) || myMetadataFiles.contains(name)) {
+ continue;
+ }
+
+ String otherUuid = readTableUuid(table, info.location());
+ if (otherUuid == null || !otherUuid.equals(myUuid)) {
Review Comment:
(blocking) A table created with `register_table` from another table's
metadata keeps that table's uuid (`registerTable` commits the parsed metadata
as-is, and later commits keep `base.uuid`). That is the "created from another's
metadata directory" case in the PR description, and this check returns `false`
for it, so the other table's newer files would still be deleted.
--
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]
