Skip to content

Conversation

@hemanthboyina
Copy link

compute_table_stats fails with CommitFailedException when concurrent writes modify table metadata during statistics computation.

@github-actions github-actions bot added the core label Jan 26, 2026
Copy link
Contributor

@dramaticlly dramaticlly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we shall add commit retry like other PendingUpdate implementation class, RemoveSnapshots is a good example. But we can probably simplify the unit test a bit

Copy link
Contributor

@dramaticlly dramaticlly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dramaticlly
Copy link
Contributor

@huaxingao @nastra I think this one is close, can you take a look as well?

2.0 /* exponential */)
.onlyRetryOn(CommitFailedException.class)
.run(
item -> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: item is not used

TableMetadata newMetadata = internalApply(base);
ops.commit(base, newMetadata);
Tasks.foreach(ops)
.retry(base.propertyAsInt(COMMIT_NUM_RETRIES, COMMIT_NUM_RETRIES_DEFAULT))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

retry config is pulled from base initialized at construction time. If table properties change between construction and commit(), retries won’t pick that up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of adding a member variable base, could we simplify by using local base/updated variables per retry attempt (base = taskOps.refresh(); updated = internalApply(base); taskOps.commit(base, updated))?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants