Skip to content

KAFKA-8351; Cleaner should handle transactions spanning multiple segments#6722

Merged
hachikuji merged 4 commits into
apache:trunkfrom
hachikuji:KAFKA-8351-POSTFIX
May 25, 2019
Merged

KAFKA-8351; Cleaner should handle transactions spanning multiple segments#6722
hachikuji merged 4 commits into
apache:trunkfrom
hachikuji:KAFKA-8351-POSTFIX

Conversation

@hachikuji

Copy link
Copy Markdown
Contributor

When cleaning transactional data, we need to keep track of which transactions still have data associated with them so that we do not remove the markers. We had logic to do this, but it was not being carried over when beginning cleaning for a new set of segments. This could cause the cleaner to incorrectly believe a transaction marker was no longer needed. The fix here carries the transactional state between groups of segments to be cleaned.

Committer Checklist (excluded from commit message)

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

@dhruvilshah3 dhruvilshah3 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the patch. LGTM, left couple of minor comments.

Comment thread core/src/main/scala/kafka/log/LogCleaner.scala Outdated
Comment thread core/src/test/scala/unit/kafka/log/LogCleanerTest.scala Outdated

@viktorsomogyi viktorsomogyi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't see any problem with this fix but have some further thoughts:

  • What if the compaction policy is "delete"?
  • Are transaction markers kept in sync between user topics and __consumer_offsets and __transaction_state with regard to cleaning?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: might be better to use addAbortedTransactions.

@junrao junrao left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@hachikuji : Thanks for the PR. LGTM. Just a minor comment below.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we add transactionMetadata to the javadoc?

@hachikuji
hachikuji force-pushed the KAFKA-8351-POSTFIX branch from df5c392 to e525b63 Compare May 24, 2019 23:24
@hachikuji

Copy link
Copy Markdown
Contributor Author

@viktorsomogyi Thanks for the questions.

What if the compaction policy is "delete"?

Just like a normal topic, producer state can be lost due to retention. Note that I have proposed in KIP-360 to retain producer state beyond the retention in the log. That may allow us simplify the cleaner's bookkeeping here.

Are transaction markers kept in sync between user topics and __consumer_offsets and __transaction_state with regard to cleaning?

I'm not 100% sure I understand the question, but transaction state is local to the partition after the transaction completes. Once markers are written to the log, the transaction is considered complete locally and the data is exposed.

@hachikuji
hachikuji merged commit 6afb0ca into apache:trunk May 25, 2019
hachikuji added a commit that referenced this pull request May 25, 2019
…ents (#6722)

When cleaning transactional data, we need to keep track of which transactions still have data associated with them so that we do not remove the markers. We had logic to do this, but the state was not being carried over when beginning cleaning for a new set of segments. This could cause the cleaner to incorrectly believe a transaction marker was no longer needed. The fix here carries the transactional state between groups of segments to be cleaned.

Reviewers: Dhruvil Shah <dhruvil@confluent.io>, Viktor Somogyi <viktorsomogyi@gmail.com>, Jun Rao <junrao@gmail.com>
hachikuji added a commit that referenced this pull request May 25, 2019
…ents (#6722)

When cleaning transactional data, we need to keep track of which transactions still have data associated with them so that we do not remove the markers. We had logic to do this, but the state was not being carried over when beginning cleaning for a new set of segments. This could cause the cleaner to incorrectly believe a transaction marker was no longer needed. The fix here carries the transactional state between groups of segments to be cleaned.

Reviewers: Dhruvil Shah <dhruvil@confluent.io>, Viktor Somogyi <viktorsomogyi@gmail.com>, Jun Rao <junrao@gmail.com>
haidangdam pushed a commit to haidangdam/kafka that referenced this pull request May 29, 2019
…ents (apache#6722)

When cleaning transactional data, we need to keep track of which transactions still have data associated with them so that we do not remove the markers. We had logic to do this, but the state was not being carried over when beginning cleaning for a new set of segments. This could cause the cleaner to incorrectly believe a transaction marker was no longer needed. The fix here carries the transactional state between groups of segments to be cleaned.

Reviewers: Dhruvil Shah <dhruvil@confluent.io>, Viktor Somogyi <viktorsomogyi@gmail.com>, Jason Gustafson <jason@confluent.io>
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
…ents (apache#6722)

When cleaning transactional data, we need to keep track of which transactions still have data associated with them so that we do not remove the markers. We had logic to do this, but the state was not being carried over when beginning cleaning for a new set of segments. This could cause the cleaner to incorrectly believe a transaction marker was no longer needed. The fix here carries the transactional state between groups of segments to be cleaned.

Reviewers: Dhruvil Shah <dhruvil@confluent.io>, Viktor Somogyi <viktorsomogyi@gmail.com>, Jason Gustafson <jason@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants