Skip to content

Feature: Rollback compaction on conflict #1285

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Feature: Rollback compaction on conflict #1285

wants to merge 4 commits into from
+561 −1

Conversation

singhpk234
Copy link
Collaborator

@singhpk234 singhpk234 commented Mar 31, 2025 •
edited
Loading

About The Change

Intention is make the catalog smarter, to revert the compaction commits in case of crunch to let the writers who are actually adding or removing the data to the table succeed. In a sense treating compaction as always a lower priority process.

Presently the rest catalog client creates the snapshot and asks the Rest Server to apply the snapshot and gives this in a combination of requirement and update.

Polaris could apply some basic inference and generate some updates to metadata given a property is enabled at a table level, by saying that It will revert back the commit which was created by compaction and let the write succeed.
I had this PR in OSS, which was essentially doing this at the client end, but we think its best if we do this as server end. to support more such clients.

Devlist: https://lists.apache.org/thread/8k8t77dgk1vc124fnb61932bdp9kf1lc

New Scenario :

Screenshot 2025-03-31 at 1 07 29 PM

@github-project-automation github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Mar 31, 2025
@singhpk234 singhpk234 force-pushed the feature/rollback-compac-polaris branch from 468051c to 05f9bad Compare April 3, 2025 06:44
@singhpk234 singhpk234 force-pushed the feature/rollback-compac-polaris branch 2 times, most recently from 01571dc to 285cc29 Compare April 18, 2025 07:30
@singhpk234 singhpk234 marked this pull request as ready for review April 18, 2025 07:32
@singhpk234
Copy link
Collaborator Author

cc @RussellSpitzer @flyrain

@singhpk234 singhpk234 force-pushed the feature/rollback-compac-polaris branch from 285cc29 to eb06c77 Compare April 18, 2025 16:24
eric-maynard
eric-maynard reviewed Apr 18, 2025
View reviewed changes
...e/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java

try {
Tasks.foreach(new TableOperations[] {ops})
.retry(4)
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto, can we make this configurable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added more comments on the top, idea was to not diverge in terms of diff from the impl in CatalogHandle, hence didn't make this, this is ditto copy of what we have there.

Please let me know your thoughts considering this, happy to make this configurable.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be the same as the commit-retry param? Not sure polaris needs to obey that but it does exist

...e/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java

// TODO: Clean this up when CatalogHandler become extensible.
// Copy of CatalogHandler#update
private static LoadTableResponse updateTableWithRollback(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just use baseCatalog instead of making this static?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is mostly taken CatalogHandler#updateTable and it calling CatalogHandler#commit which is protected, so they are exact same except the part of rollback-logic

Copy link
Contributor

@eric-maynard eric-maynard Apr 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I know the name is a little confusing, but I don't think we really want to copy too much CatalogHandler logic into IcebergCatalogHandler -- previously, this class was named like IcebergCatalogHandlerWrapper. It's okay to copy small methods where we need to override behavior imo, but this is a lot of logic.

I think your comment is correct though that once CatalogHandler is extensible a lot of this goes away. So maybe it's ok for now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ack, I agree, I hope this methods were public and hence extensible.

@singhpk234 singhpk234 force-pushed the feature/rollback-compac-polaris branch 3 times, most recently from 354b12b to f1de46c Compare April 22, 2025 18:36
RussellSpitzer
RussellSpitzer reviewed Apr 25, 2025
View reviewed changes
...e/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java
@@ -762,7 +777,190 @@ public LoadTableResponse updateTable(
if (isStaticFacade(catalog)) {
throw new BadRequestException("Cannot update table on static-facade external catalogs.");
}
return CatalogHandlers.updateTable(baseCatalog, tableIdentifier, applyUpdateFilters(request));
// TODO: pending discussion if table property is right way, or a writer specific knob is
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer this was a "catalog" owned table property or policy rather than something actually in the table metadata.json. It's the kind of thing I don't think is safe to allow for a generic user to alter/set

Copy link
Contributor

Choose a reason for hiding this comment

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

#1417 :)

RussellSpitzer
RussellSpitzer reviewed Apr 25, 2025
View reviewed changes
...e/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java
request.updates().forEach((update) -> update.applyTo(newMetadataBuilder));
TableMetadata updated = newMetadataBuilder.build();
// always commit this
taskOps.commit(base, updated);
Copy link
Member

Choose a reason for hiding this comment

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

I think this is an interesting approach and probably the right one, but I'm wondering if you considered just reloading the metadata.json from the time of the rollback and applying the commit to that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Posting Discussion offline : Idea was to not skip schema changes that could have potentially landed in between and for which we don't create a new snapshot we just create a new metadata.json so re-using metadata.json was something which we wanted. Though reloading the metadata.json from the time of rollback is also possible we can also checks like if current_schem_id, stats files etc are same or not to ensure we are not loosing any updates which doesn't have snapshot of their own, though this idea looks a bit easy with implementation wise hence choosing above.
TODO: think more on how else can this be achieved.

@singhpk234 singhpk234 force-pushed the feature/rollback-compac-polaris branch 2 times, most recently from b7c6d2f to aa7d04b Compare April 26, 2025 09:44
@singhpk234 singhpk234 force-pushed the feature/rollback-compac-polaris branch from aa7d04b to 0db3770 Compare April 27, 2025 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RussellSpitzer RussellSpitzer RussellSpitzer left review comments

@eric-maynard eric-maynard eric-maynard left review comments

@adutra adutra Awaiting requested review from adutra adutra is a code owner

@ashvina ashvina Awaiting requested review from ashvina ashvina is a code owner

@dennishuo dennishuo Awaiting requested review from dennishuo dennishuo is a code owner

@dimas-b dimas-b Awaiting requested review from dimas-b dimas-b is a code owner

@jackye1995 jackye1995 Awaiting requested review from jackye1995 jackye1995 is a code owner

@jbonofre jbonofre Awaiting requested review from jbonofre jbonofre is a code owner

@vvcephei vvcephei Awaiting requested review from vvcephei vvcephei is a code owner

@collado-mike collado-mike Awaiting requested review from collado-mike collado-mike is a code owner

@snazy snazy Awaiting requested review from snazy snazy is a code owner

@takidau takidau Awaiting requested review from takidau takidau is a code owner

@MonkeyCanCode MonkeyCanCode Awaiting requested review from MonkeyCanCode MonkeyCanCode is a code owner

@flyrain flyrain Awaiting requested review from flyrain flyrain is a code owner

@ebyhr ebyhr Awaiting requested review from ebyhr ebyhr is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@singhpk234 @RussellSpitzer @eric-maynard