zeroshade commented on code in PR #1292:
URL: https://github.com/apache/iceberg-go/pull/1292#discussion_r3483352284
##########
catalog/hadoop/hadoop.go:
##########
@@ -492,15 +598,6 @@ func (c *Catalog) CommitTable(ctx context.Context, ident
table.Identifier, reqs
}
// Step 6: Determine next version number.
- var currentVersion int
-
- if current != nil {
- currentVersion, err = c.findVersion(ident)
- if err != nil {
- return nil, "", err
- }
- }
-
newVersion := currentVersion + 1
Review Comment:
[Major] Reusing `currentVersion` here is the right call. The gap is
downstream: the pre-rename conflict check a few lines below
(`Stat(newMetaPath)`, where `newMetaPath` is `v{newVersion}.metadata.json`)
only probes the Hadoop-style filename. Now that this PR teaches the catalog to
*read* UUID- and gzip-named metadata, a concurrent writer (Java/PyIceberg) that
lands `%05d-<uuid>.metadata.json` or `v{newVersion}.gz.metadata.json` for the
same version won't be detected, and we can end up writing a second
`v{newVersion}.metadata.json` for the same logical version.
Suggest rescanning the metadata dir (via
`scanMetadataFiles`/`metadataFileFromName`) for any recognized file whose
parsed version == `newVersion` immediately before the rename, failing if one
exists - plus a regression test that stages a UUID/gzip next-version file
between load and commit. (HadoopCatalog isn't a substitute for a locking
catalog under concurrent multi-writer use, but this keeps the write/conflict
path consistent with the new read support.)
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
