KAFKA-10702; Skip bookkeeping of empty transactions

[hachikuji]

Compacted topics can accumulate a large number of empty transaction markers as the data from the transactions gets cleaned. For each transaction, there is some bookkeeping that leaders and followers must do to keep the transaction index up to date. The cost of this overhead can degrade performance when a replica needs to catch up if the log has mostly empty or small transactions. This patch improves the cost by skipping over empty transactions since these will have no effect on the last stable offset and do not need to be reflected in the transaction index.

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[hachikuji]

Addressing this problem more generally so that we can also handle small transactions is difficult because of the need to maintain the index. I believe there is still room for improvement by looking only at the aborted transactions, but this is more complex and I need to think it through.

[lbradstreet]

Could you check my understanding? If we have a a non-empty currentTxnFirstOffset value (indicating a non-empty transaction), we'll return a valid CompletedTxn, otherwise we will return None. For the empty transactions this means that we aren't accumulating completed transactions. This saves us from having to call lastStableOffset on every empty completed transaction https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/log/Log.scala#L1240?

[hachikuji]

Yes, that is right. Additionally, we are not adding the transaction to the list of started transactions which are accumulated in the ProducerAppendInfo.

[hachikuji]

Note that this change is causing LogTest.testAppendToTransactionIndexFailure to fail. I think this has surfaced an inconsistency in how we update the transaction state. I'm considering the best way to resolve it.

[lbradstreet]

Note that this change is causing LogTest.testAppendToTransactionIndexFailure to fail. I think this has surfaced an inconsistency in how we update the transaction state. I'm considering the best way to resolve it.

The changes appear to be good to me and resolves the performance issue. I'll hold off on a final verdict until you've investigated the failure.

[hachikuji]

For a little more background about the LogTest.testAppendToTransactionIndexFailure failure, it is due to an inconsistency in how we update state in ProducerStateManager. The current append flow is the following:

1. Build producer state in ProducerAppendInfo instances and collect completed transactions
2. Append the entry to the log
3. Update log end offset
4. Apply individual producer state to ProducerStateManager
5. Update the transaction index
6. Update completed transactions and advance LSO

The idea is that the LSO is stuck if an append to the transaction index fails. However, because we have already updated producer state before the index write, we are left with an inconsistency. The LSO will reflect an ongoing transaction which is not reflected in any of the producer states.

The test case that is failing is validating the behavior when the index write fails. It works like this:

1. First append some transactional data to the log
2. Append an ABORT marker, but let the write to the transaction index fail
3. Retry the append of the ABORT and verify that append still fails and the LSO is stuck

The test fails because the second append no longer attempts to write to the transaction index. I can change the test of course, but I was disturbed about the underlying assumption that the write of the transaction marker can be retried on the Log after a failure. In fact, the path to fencing the Log after a write failure is asynchronous today. We use LogDirFailureChannel to propagate log failures to a separate thread which is responsible for marking the log dir offline or shutting down the broker. So there is indeed a (small) window during which a WriteTxnMarkers request could be retried. My feeling is that EOS demands a stronger guarantee and we need to fence off the Log instance synchronously while still holding the lock.

So I think we need a separate jira to fix this issue. The question then is whether it should block this patch or not. I am thinking not at the moment. The test fails because there is no second append to the transaction index, but this is not required for correctness, and the LSO remains stuck as expected in any case. Basically I'd say we're no worse than before. I will add a commit which alters the test case so that it can pass and we can discuss tightening up the failure logic in a separate jira.

[hachikuji]

Note I filed https://issues.apache.org/jira/browse/KAFKA-10778 to introduce synchronous log fencing after IO errors.

[junrao]

@hachikuji : Thanks for the PR. LGTM