zeroshade commented on code in PR #1326:
URL: https://github.com/apache/iceberg-go/pull/1326#discussion_r3521466790
##########
catalog/hadoop/hadoop.go:
##########
@@ -547,17 +584,27 @@ func (c *Catalog) CommitTable(ctx context.Context, ident
table.Identifier, reqs
return nil, "", fmt.Errorf("hadoop catalog: failed to create
metadata directory: %w", err)
}
- newMetaPath := c.metadataFilePath(ident, newVersion)
- tempPath := joinPath(c.isLocal, metaDir,
uuid.New().String()+".metadata.json")
-
compression := updated.Properties().Get(table.MetadataCompressionKey,
table.MetadataCompressionDefault)
+ newMetaPath, err := c.metadataFilePathForCompression(ident, newVersion,
compression)
+ if err != nil {
+ return nil, "", fmt.Errorf("hadoop catalog: failed to determine
metadata file path: %w", err)
+ }
+
+ tempPath := joinPath(c.isLocal, metaDir,
uuid.New().String()+".metadata.json")
if err := internal.WriteTableMetadata(updated, c.filesystem, tempPath,
compression); err != nil {
_ = c.filesystem.Remove(tempPath)
return nil, "", fmt.Errorf("hadoop catalog: failed to write
table metadata: %w", err)
}
+ if existingPath, exists := c.metadataVersionLocation(ident,
newVersion); exists {
+ _ = c.filesystem.Remove(tempPath)
Review Comment:
[MAJOR] The cross-codec commit conflict check is non-atomic. Two concurrent
writers can both compute the same newVersion, both pass
metadataVersionLocation(ident, newVersion) before either publishes, then one
RenameNoReplace can publish vN.metadata.json while the other publishes
vN.gz.metadata.json; both succeed, leaving two files claiming the same version.
Fix by taking an atomic per-version claim independent of codec suffix before
publishing (for example a RenameNoReplace sentinel for vN), and add a
concurrent mixed-codec commit test where both writers pass preflight before
either final rename.
##########
catalog/hadoop/hadoop.go:
##########
@@ -424,16 +459,20 @@ func (c *Catalog) CreateTable(ctx context.Context, ident
table.Identifier, sc *i
}
version := 1
- metaPath := c.metadataFilePath(ident, version)
- tempPath := joinPath(c.isLocal, metaDir,
uuid.New().String()+".metadata.json")
-
compression := table.MetadataCompressionDefault
if cfg.Properties != nil {
if v, ok := cfg.Properties[table.MetadataCompressionKey]; ok {
compression = v
}
}
+ metaPath, err := c.metadataFilePathForCompression(ident, version,
compression)
Review Comment:
[MAJOR] CreateTable has the same duplicate-version race with different
compression. Two concurrent creators can both pass isTableDir, then publish
v1.metadata.json and v1.gz.metadata.json successfully. Reuse a
codec-independent per-version/table-creation claim before publishing metadata,
and add a concurrent create test with default and gzip writers.
--
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]
