HIVE-29420: Hive ACID: Cleaner mishandles retries of killed compactions

[deniskuzZ]

What changes were proposed in this pull request?

1. Replaced assert with an exception in TxnUtils#createValidTxnListForCleaner
2. Fixed findReadyToClean eligibility condition in ReadyToCleanHandler
   (i.e. CQ_HIGHEST_WRITE_ID < MIN_OPEN_WRITE_ID - 1)
3. Removed redundant addWriteIdsToMinHistory from AcidCompactionService

Why are the changes needed?

Compaction retries triggered by timeouts under an incorrect configuration pose a risk of data loss. If a compaction is re-attempted before the prior attempt has completed or been properly aborted, the Cleaner may observe multiple base directories with the same writeId and erroneously delete all of them.

Does this PR introduce any user-facing change?

No

How was this patch tested?

TestCleanerWithMinHistoryWriteId

[kuczoram]

Thanks a lot Denys for this patch! Overall it looks good, the test cases also look good. The second testcase indeed can reproduce the issue with the two bases (if the assert is disabled).
I have one question about the SQL query modification.

[kuczoram]

Why do you need the -1 here? Could you please explain this a little?

[deniskuzZ]

sure.

Scenario

1. Multiple deltas exist with max writeId = 50
2. SELECT query (TXN_ID=100) starts:

• Takes snapshot, sees writeId 50 as committed;
• Records minOpenWriteId = 51;
• Begins reading from deltas up to writeId 50;

3. Compaction (TXN_ID=101) runs:

• Produces base_50 (includes all data up to writeId 50);
• Does not create new writeIds;
• Commits;

4. Cleaner starts and evaluates cleanup eligibility

Without the -1 offset
Condition: CQ_HIGHEST_WRITE_ID < MIN_OPEN_WRITE_ID

• 50 < 51 = true → cleaner proceeds
• Problem: The SELECT query (TXN_ID=100) is still running and reading from deltas
• The reader cannot switch to base_50 mid-query; its snapshot was taken before compaction committed
• Cleaner throws an exception when evaluating createValidTxnListForCleaner (newly added guard) with something like (txnid:100 is open and <= hwm: 50).
• It may retry several times and potentially mark the cleanup attempt as failed.

With the -1 offset
Condition: CQ_HIGHEST_WRITE_ID < MIN_OPEN_WRITE_ID - 1

• MIN_OPEN_WRITE_ID - 1 => Highest write ID for a compacted file to be considered by reader;
• 50 < 51 - 1 = 50 < 50 = false → cleaner does not proceed
• This requires a gap of at least 1 writeId between compaction's HWM and the minimum open writeId

The -1 offset protects long-running readers that started before compaction committed by ensuring cleanup only proceeds when there's a writeId gap, indicating it's safe to delete the obsolete deltas.

PS: We could have applied the CQ_NEXT_TXN_ID < MIN_OPEN_TXN_ID condition; however, doing so would require synchronization/locking during transaction commit to obtain an accurate CQ_NEXT_TXN_ID.

[sonarqubecloud]

Quality Gate passed

Issues
46 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.4% Duplication on New Code

See analysis details on SonarQube Cloud