JingsongLi commented on code in PR #9245:
URL: https://github.com/apache/paimon/pull/9245#discussion_r3802142967
##########
paimon-iceberg/src/main/java/org/apache/paimon/iceberg/IcebergRestMetadataCommitter.java:
##########
@@ -151,6 +156,14 @@ private void commitMetadataImpl(
try {
if (!tableExists()) {
+ if (newMetadata.currentSnapshot() != null) {
+ // metadata with history is imported wholesale: replaying
it through
+ // TableMetadata.Builder would recompute next-row-id from
added-rows
+ // and lose any high-water mark that does not start at 0
+ LOG.info("Table {} does not exist, register it.",
icebergTableIdentifier);
+ registerAsCurrent(newIcebergMetadata, newMetadata, false);
Review Comment:
[P1] Do not require RegisterTable from every REST catalog. This changes
every first publication that already has a snapshot, not only v3 recovery, to
use registerTable. RegisterTable is not implemented by every REST service that
supports the previous create/update flow; for example, AWS Glue documents
CreateTable, LoadTable, UpdateTable, and DeleteTable, but not RegisterTable
(https://docs.aws.amazon.com/glue/latest/dg/connect-glu-iceberg-rest.html). The
Apache test server advertises this endpoint, so the new tests cannot catch that
compatibility regression. Please check the server-advertised endpoint
capabilities: keep the create/update path for v2 and zero-based initial v3
metadata when registration is unavailable, and fail closed before any drop when
a nonzero watermark can only be preserved through registration.
##########
paimon-iceberg/src/main/java/org/apache/paimon/iceberg/IcebergRestMetadataCommitter.java:
##########
@@ -321,6 +338,54 @@ void createDatabase() {
}
}
+ /**
+ * Makes the catalog's state exactly the (REST-adjusted) local metadata by
registering a
+ * metadata file, instead of rebuilding the table through {@link
TableMetadata.Builder}. The
+ * builder recomputes the row-id high-water mark from each snapshot's
added-rows, so any id
+ * space that does not start at 0 (self-heal and rollback rebuilds) would
end below the ids
+ * already assigned in manifests, and a later external writer could reuse
them. Registration
+ * imports every field of the metadata verbatim, next-row-id included.
+ */
+ private void registerAsCurrent(
+ IcebergMetadata adjustedMetadata, TableMetadata newMetadata,
boolean dropFirst) {
+ try {
+ Path registerPath =
+ new Path(
+ tableLocation,
+ String.format(
+ "metadata/rest-register-v%d.metadata.json",
+ adjustedMetadata.currentSnapshotId()));
+ if (!fileIO.tryToWriteAtomic(registerPath,
adjustedMetadata.toJson())) {
+ fileIO.deleteQuietly(registerPath);
Review Comment:
[P1] Never overwrite a metadata file that may already be registered. The
filename is derived only from the numeric Paimon snapshot ID, but
rollback/rebuild paths can reuse that ID for a different timeline. On the
second registration, this branch deletes and overwrites the old file before the
catalog pointer is changed; a catalog that registers the supplied location by
reference can therefore expose different table metadata without a catalog
commit, and a failed drop/register can leave its current pointer referencing
mutated content. Iceberg requires written metadata files to remain immutable
and commits to publish a new unique file via an atomic pointer swap
(https://iceberg.apache.org/spec/#file-system-operations). Please use a
write-once UUID, timeline, or content-hash filename for every registration
attempt, validate it, and never delete or overwrite a prior registration file.
--
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]
