tanmayrauth commented on code in PR #1338:
URL: https://github.com/apache/iceberg-go/pull/1338#discussion_r3486694622
##########
catalog/glue/glue.go:
##########
@@ -461,29 +461,44 @@ func (c *Catalog) RenameTable(ctx context.Context, from,
to table.Identifier) (*
return nil, fmt.Errorf("failed to create the table %s.%s: %w",
fromDatabase, fromTable, err)
}
+ currentFromGlueTable, err := c.getTable(ctx, fromDatabase, fromTable)
+ if err != nil {
+ c.rollbackRenamedTable(ctx, toDatabase, toTable)
+
+ return nil, fmt.Errorf("failed to revalidate the table %s.%s
before delete: %w", fromDatabase, fromTable, err)
+ }
+ if glueTableChangedDuringRename(fromGlueTable, currentFromGlueTable) {
+ c.rollbackRenamedTable(ctx, toDatabase, toTable)
+
+ return nil, fmt.Errorf("failed to rename the table %s.%s:
source table changed during rename", fromDatabase, fromTable)
+ }
+
// Drop the old table.
_, err = c.glueSvc.DeleteTable(ctx, &glue.DeleteTableInput{
CatalogId: c.catalogId,
DatabaseName: aws.String(fromDatabase),
Name: aws.String(fromTable),
})
if err != nil {
- // Best-effort rollback the table creation.
- _, rollbackErr := c.glueSvc.DeleteTable(ctx,
&glue.DeleteTableInput{
- CatalogId: c.catalogId,
- DatabaseName: aws.String(toDatabase),
- Name: aws.String(toTable),
- })
- if rollbackErr != nil {
- fmt.Printf("failed to rollback the new table %s.%s:
%v", toDatabase, toTable, rollbackErr)
- }
+ c.rollbackRenamedTable(ctx, toDatabase, toTable)
return nil, fmt.Errorf("failed to rename the table %s.%s: %w",
fromDatabase, fromTable, err)
}
return c.LoadTable(ctx, to)
}
+func (c *Catalog) rollbackRenamedTable(ctx context.Context, database,
tableName string) {
+ _, rollbackErr := c.glueSvc.DeleteTable(ctx, &glue.DeleteTableInput{
+ CatalogId: c.catalogId,
+ DatabaseName: aws.String(database),
+ Name: aws.String(tableName),
+ })
+ if rollbackErr != nil {
+ fmt.Printf("failed to rollback the new table %s.%s: %v",
database, tableName, rollbackErr)
Review Comment:
nit: the Printf on rollback failure is pre-existing code you've just
relocated, so not a blocker but since it's moving anyway, worth routing through
a logger and adding the missing trailing newline.
##########
catalog/glue/glue.go:
##########
@@ -461,29 +461,44 @@ func (c *Catalog) RenameTable(ctx context.Context, from,
to table.Identifier) (*
return nil, fmt.Errorf("failed to create the table %s.%s: %w",
fromDatabase, fromTable, err)
}
+ currentFromGlueTable, err := c.getTable(ctx, fromDatabase, fromTable)
Review Comment:
This closes most of the race, but there's still a TOCTOU gap between this
revalidation GetTable and the DeleteTable below — a writer that commits in that
sub-window still loses its commit. The summary already notes Glue has no
conditional DeleteTable, so I think this is the best available mitigation (and
create-then-delete is the right order). Could you add a line to the comment
making clear it narrows rather than eliminates the race? Just so a future
reader doesn't assume it's airtight.
##########
catalog/glue/glue.go:
##########
@@ -843,3 +858,25 @@ func constructDatabaseInput(database string, props
iceberg.Properties) *types.Da
func isConcurrentModificationException(err error) bool {
return errors.As(err, new(*types.ConcurrentModificationException))
}
+
+func glueTableChangedDuringRename(original, current *types.Table) bool {
+ if original == nil || current == nil {
+ return true
+ }
+
+ originalVersionID := aws.ToString(original.VersionId)
+ currentVersionID := aws.ToString(current.VersionId)
+ if originalVersionID != "" || currentVersionID != "" {
Review Comment:
The version-first / metadata-location-fallback logic looks right to me,
VersionId bumps on every Glue UpdateTable, so it's strictly stronger than
comparing metadata_location, and only falling back when both
versions are empty is sensible. This does rely on two back-to-back
GetTable calls returning the same VersionId absent a write (true for Glue), so
no spurious rename failures — worth a one-line comment to
capture that assumption.
--
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]
