HIVE-29572: ACID Compaction: Cleaner should mark a compaction failed when its txn is aborted

[kuczoram]

What changes were proposed in this pull request?

When a compaction is fetched by the cleaner, first check the state of the compaction's transaction. If it is already aborted, delete nothing, but mark the compaction as failed.

Why are the changes needed?

It can happen that a compaction is marked as finished and get into "ready for cleaning" state, but the compaction txn stays open. And when the timeout reached, the txn gets aborted.
With min.history.level, a compaction like this can block the cleaning for all consecutive compaction, even after the txn is aborted.

Does this PR introduce any user-facing change?

No

How was this patch tested?

The scenarios are covered in TestCleaner unit test.

[sonarqubecloud]

Quality Gate passed

Issues
17 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

[deniskuzZ]

i don't think that is an optimal solution.
if i get it right, it requires checking every cleanup request for an open txn and creates additional stress on backend DB

[deniskuzZ]

It can happen that a compaction is marked as finished and get into "ready for cleaning" state, but the compaction txn stays open. And when the timeout reached, the txn gets aborted.

could we handle this in abortTxn? if txnType=3 (compaction) cleanup the COMPACTION_QUEUE as well?

[deniskuzZ]

With min.history.level, a compaction like this can block the cleaning for all consecutive compaction, even after the txn is aborted.

if i get it right, it's not the case with min.history.writeid, however, the check is generic

[kuczoram]

It checks for the transaction state only for compactions which don't have nextTxnId. If I understand it correctly if a compaction's txn is committed successfully, the nextTxnId is filled. So it will mean just one additional HMS call for compactions without successfully committed txn. For compaction's with comitted txn, it will do nothing.

I tried an other approach as well, when I made the compactions failed in the AbortTxnsFunction, but I didn't really like that either. I didn't find a way to call the markFailed from there, so I either had to extend the SQLs there or fetch the compactioninfo for all txn ids and then check if the condition's are matched. Like it has to be a compaction, not a soft-delete and it has to be in ready-for-cleaning state, because if it is in working state, we cannot fail it, as it could be revoked. At the end I felt that going with this approach could have more side effects. So I went with this one, but I can go back to making the failure in AbortTxnsFunction if you'd like that.

[kuczoram]

With min.history.level, a compaction like this can block the cleaning for all consecutive compaction, even after the txn is aborted.

if i get it right, it's not the case with min.history.writeid, however, the check is generic

No, with min.history.writeid it is not blocking the following cleaners. Because in that case what happens is that the cleaner's highwatermark will be 0, so it won't delete anything, but there is no remainder checking for min.history.writeid, so it will consider the cleaning as successful. Marks the compaction as successful and just goes on. With min.history.level, it will clean nothing, but finds deltas which should be deleted, so it will stay in the queue with "ready for cleaning" state.

[deniskuzZ]

If I understand it correctly if a compaction's txn is committed successfully, the nextTxnId is filled.

why uncommited txn would be even eligible in first place? is it a race between mark ready-for-cleaning and commit/abort? what if we mark after commit/abort or make commit atomic?