Skip to content

HIVE-29571: ACID Compaction: Marking the compaction as compacted should happen after its txn got committed#6511

Open
kuczoram wants to merge 3 commits into
apache:masterfrom
kuczoram:HIVE-29571
Open

HIVE-29571: ACID Compaction: Marking the compaction as compacted should happen after its txn got committed#6511
kuczoram wants to merge 3 commits into
apache:masterfrom
kuczoram:HIVE-29571

Conversation

@kuczoram
Copy link
Copy Markdown
Contributor

@kuczoram kuczoram commented May 27, 2026

What changes were proposed in this pull request?

Change how the compaction gets committed by first committing its txn and just after that mark the compaction as ready for cleaning.

Why are the changes needed?

Currently the markCompacted call and the commitTxn call happens separately. The markCompacted call happens in the AcidCompactionService.compact method while the commitTxn happens in the close() method. This could lead to inconsistent state that the compaction is marked as finished and ready for cleaning, but the compaction transaction is still open.

Does this PR introduce any user-facing change?

No

How was this patch tested?

By unit tests
Ran the following unit tests against the master branch as well to see if there is no regression in these use-cases. They should not be affected by the changes in this patch.

  • testExceptionDuringCompact
  • testWorkerIfIsDynPartAbort
  • testWorkerNotEnoughToCompact
  • testWorkerNotEnoughToCompactNeedsCleaning

commit();
} catch (Exception e) {
abort();
if (onAbortAction != null) onAbortAction.run();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i think we should have default onAbortAction = msc.markFailed(CompactionInfo.compactionInfoToStruct(ci))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

onAbortAction here would be probably null

…ld happen after its txn got committed - made some fixes and added more tests
}

void markForAbort(ThrowingRunnable action) {
this.rollbackOnly = true;
Copy link
Copy Markdown
Member

@deniskuzZ deniskuzZ May 28, 2026

Choose a reason for hiding this comment

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

this might be redundant since we have default

Copy link
Copy Markdown
Member

@deniskuzZ deniskuzZ left a comment

Choose a reason for hiding this comment

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

LGTM +1, minor comment

@sonarqubecloud
Copy link
Copy Markdown

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants