tanmayrauth commented on code in PR #1467:
URL: https://github.com/apache/iceberg-go/pull/1467#discussion_r3592556776
##########
catalog/glue/glue.go:
##########
@@ -443,37 +447,48 @@ func (c *Catalog) RenameTable(ctx context.Context, from,
to table.Identifier) (*
if err != nil {
return nil, fmt.Errorf("failed to fetch the table %s.%s: %w",
fromDatabase, fromTable, err)
}
+ if !strings.EqualFold(fromGlueTable.Parameters[tableParamTableType],
glueTypeIceberg) {
Review Comment:
getTable at line 446 already errors on non-iceberg (and RENAMING) tables, so
this branch is unreachable. Not harmful, but it reads as if getTable can return
non-iceberg tables — drop it or add a comment.
##########
catalog/glue/glue.go:
##########
@@ -671,6 +754,23 @@ func (c *Catalog) ListNamespaces(ctx context.Context,
parent table.Identifier) (
// GetTable loads a table from the Glue Catalog using the given database and
table name.
func (c *Catalog) getTable(ctx context.Context, database, tableName string)
(*types.Table, error) {
+ tbl, err := c.getRawTable(ctx, database, tableName)
+ if err != nil {
+ return nil, err
+ }
+
+ tableType := tbl.Parameters[tableParamTableType]
+ if tableType == glueTypeIcebergRenaming {
+ return nil, fmt.Errorf("%w: table %s.%s is being renamed",
table.ErrCommitFailed, database, tableName)
Review Comment:
This makes getTable reject any ICEBERG_RENAMING table, and every public op
goes through getTable — LoadTable (236), CommitTable (317), DropTable (387),
and RenameTable's own source fetch (446). So if the process dies between the
successful claim (line 472) and the delete/rollback, the source is stuck in
ICEBERG_RENAMING and nothing — load, commit, drop, or re-rename — can recover
it; the only fix is manual Glue surgery. That's a regression from the old path,
which never mutated the source, so a crash always left it usable. Worst case:
claim succeeds, pod is killed before DeleteTable, and on restart
DropTable(from) returns "is being renamed" with no way to clean up. Suggest
DropTable go through getRawTable and be allowed to delete a claimed source so
there's an in-API recovery path.
##########
catalog/glue/glue.go:
##########
@@ -443,37 +447,48 @@ func (c *Catalog) RenameTable(ctx context.Context, from,
to table.Identifier) (*
if err != nil {
return nil, fmt.Errorf("failed to fetch the table %s.%s: %w",
fromDatabase, fromTable, err)
}
+ if !strings.EqualFold(fromGlueTable.Parameters[tableParamTableType],
glueTypeIceberg) {
+ return nil, fmt.Errorf("failed to rename the table %s.%s:
source is not an iceberg table", fromDatabase, fromTable)
+ }
+ if aws.ToString(fromGlueTable.VersionId) == "" {
+ return nil, fmt.Errorf("failed to rename the table %s.%s: Glue
table version id is missing", fromDatabase, fromTable)
+ }
// Create the new table.
_, err = c.glueSvc.CreateTable(ctx, &glue.CreateTableInput{
CatalogId: c.catalogId,
DatabaseName: aws.String(toDatabase),
- TableInput: &types.TableInput{
- Name: aws.String(toTable),
- TableType: fromGlueTable.TableType,
- Owner: fromGlueTable.Owner,
- Description: fromGlueTable.Description,
- Parameters: fromGlueTable.Parameters,
- StorageDescriptor: fromGlueTable.StorageDescriptor,
- },
+ TableInput: glueTableInput(toTable, fromGlueTable),
})
if err != nil {
return nil, fmt.Errorf("failed to create the table %s.%s: %w",
fromDatabase, fromTable, err)
}
- // Revalidate the source table immediately before delete to narrow the
- // rename race window. Glue does not offer a conditional DeleteTable, so
- // this cannot eliminate the race if a commit lands after this read.
- currentFromGlueTable, err := c.getTable(ctx, fromDatabase, fromTable)
+ // Claim the source with a conditional update before issuing Glue's
+ // unconditional delete. Changing table_type makes new Iceberg writers
refuse
+ // the source, while VersionId rejects writers that loaded the previous
version.
+ renameToken := fmt.Sprintf("%s.%s@%s", toDatabase, toTable,
aws.ToString(fromGlueTable.VersionId))
+ claimInput := glueTableInput(fromTable, fromGlueTable)
+ claimInput.Parameters[tableParamTableType] = glueTypeIcebergRenaming
Review Comment:
This marker is durable and there's no self-healing for it — the rename-token
embeds only dest+VersionId, no timestamp, so a stale claim from a crashed
rename is indistinguishable from a live one. Worth either stamping a timestamp
so a recovery path can age out stale claims, or documenting that operators must
clear the marker manually.
--
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]
