Skip to content

KAFKA-14558: Move/Rewrite LastRecord, TxnMetadata, BatchMetadata, ProducerStateEntry, and ProducerAppendInfo to the storage module#13043

Merged
ijuma merged 11 commits intoapache:trunkfrom
satishd:mv-producer-statemanager-auxiliary-classes
Jan 8, 2023
Merged

KAFKA-14558: Move/Rewrite LastRecord, TxnMetadata, BatchMetadata, ProducerStateEntry, and ProducerAppendInfo to the storage module#13043
ijuma merged 11 commits intoapache:trunkfrom
satishd:mv-producer-statemanager-auxiliary-classes

Conversation

@satishd
Copy link
Copy Markdown
Member

@satishd satishd commented Dec 23, 2022 •
edited by ijuma
Loading

For broader context on this change, you may want to look at KAFKA-14470: Move log layer to storage module.

Committer Checklist (excluded from commit message)

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

ijuma
ijuma reviewed Dec 23, 2022
View reviewed changes
Comment thread core/src/main/scala/kafka/log/LogCleaner.scala Outdated
@satishd satishd force-pushed the mv-producer-statemanager-auxiliary-classes branch 2 times, most recently from b00e944 to 2c7a350 Compare December 29, 2022 12:39
@satishd satishd changed the title [Draft] Move ProducerStateManager auxiliary classes to storage module. KAFKA-14558 Move LastRecord, TxnMetadata, BatchMetadata, ProducerStateEntry, and ProducerAppendInfo to the storage module. Dec 30, 2022
@satishd satishd force-pushed the mv-producer-statemanager-auxiliary-classes branch from 2c7a350 to 4eef3a2 Compare December 30, 2022 06:23
@satishd satishd marked this pull request as ready for review December 30, 2022 06:24
@satishd satishd marked this pull request as draft December 30, 2022 06:25
@satishd satishd force-pushed the mv-producer-statemanager-auxiliary-classes branch from 4eef3a2 to 4316d11 Compare December 30, 2022 07:27
@satishd satishd marked this pull request as ready for review December 30, 2022 08:03
@satishd satishd force-pushed the mv-producer-statemanager-auxiliary-classes branch from 4316d11 to 72f2872 Compare December 31, 2022 13:10
ijuma
ijuma reviewed Jan 2, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@ijuma ijuma left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Please see my comments below.

Comment thread core/src/main/scala/kafka/log/UnifiedLog.scala Outdated
Comment thread core/src/main/scala/kafka/log/UnifiedLog.scala Outdated
Comment thread core/src/test/scala/unit/kafka/log/LogSegmentTest.scala Outdated
Comment thread storage/src/main/java/org/apache/kafka/server/log/internals/TxnMetadata.java Outdated
Comment thread storage/src/main/java/org/apache/kafka/server/log/internals/TxnMetadata.java Outdated
Comment thread storage/src/main/java/org/apache/kafka/server/log/internals/ProducerAppendInfo.java Outdated
Comment thread storage/src/main/java/org/apache/kafka/server/log/internals/ProducerStateEntry.java Outdated
Comment thread storage/src/main/java/org/apache/kafka/server/log/internals/ProducerStateEntry.java Outdated
Comment thread storage/src/main/java/org/apache/kafka/server/log/internals/ProducerStateEntry.java Outdated
Comment thread storage/src/main/java/org/apache/kafka/server/log/internals/ProducerStateEntry.java Outdated
@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jan 4, 2023

@satishd Just checking that you saw the review for this PR (there was an issue with the notifications in a previous one, I believe). I was planning to merge this before reviewing the next one.

@satishd satishd force-pushed the mv-producer-statemanager-auxiliary-classes branch from f41ef30 to 060de9e Compare January 5, 2023 01:13
@satishd
Copy link
Copy Markdown
Member Author

satishd commented Jan 5, 2023 •
edited
Loading

@ijuma I rebased it with trunk to resolve the conflicts, working on addressing your review comments. I will let you know once I am done with my changes.

@ijuma ijuma changed the title KAFKA-14558 Move LastRecord, TxnMetadata, BatchMetadata, ProducerStateEntry, and ProducerAppendInfo to the storage module. KAFKA-14558: Move LastRecord, TxnMetadata, BatchMetadata, ProducerStateEntry, and ProducerAppendInfo to the storage module. Jan 5, 2023
@ijuma ijuma changed the title KAFKA-14558: Move LastRecord, TxnMetadata, BatchMetadata, ProducerStateEntry, and ProducerAppendInfo to the storage module. KAFKA-14558: Move LastRecord, TxnMetadata, BatchMetadata, ProducerStateEntry, and ProducerAppendInfo to the storage module Jan 5, 2023
@satishd satishd force-pushed the mv-producer-statemanager-auxiliary-classes branch from 7edd1e9 to 7a1f9b9 Compare January 5, 2023 09:14
@satishd satishd force-pushed the mv-producer-statemanager-auxiliary-classes branch from 7a1f9b9 to d181994 Compare January 5, 2023 15:54
ijuma
ijuma reviewed Jan 5, 2023
View reviewed changes
Comment thread storage/src/main/java/org/apache/kafka/server/log/internals/ProducerStateEntry.java Outdated
@satishd satishd force-pushed the mv-producer-statemanager-auxiliary-classes branch from d181994 to 89e2ec3 Compare January 5, 2023 16:32
@satishd
Copy link
Copy Markdown
Member Author

satishd commented Jan 6, 2023

Thanks @ijuma for the review. I address them in the code and replied to your comment.

ijuma
ijuma reviewed Jan 6, 2023
View reviewed changes
Comment thread storage/src/main/java/org/apache/kafka/server/log/internals/ProducerStateEntry.java Outdated
ijuma
ijuma reviewed Jan 6, 2023
View reviewed changes
Comment thread core/src/main/scala/kafka/log/LogCleaner.scala Outdated
Comment thread storage/src/main/java/org/apache/kafka/server/log/internals/ProducerAppendInfo.java Outdated
ijuma
ijuma reviewed Jan 6, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@ijuma ijuma left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. This is pretty close. Left a few more comments. I resolved all the items that were already addressed.

Comment thread core/src/test/scala/unit/kafka/log/ProducerStateManagerTest.scala Outdated
Comment thread core/src/test/scala/unit/kafka/log/ProducerStateManagerTest.scala
Comment thread core/src/test/scala/unit/kafka/log/ProducerStateManagerTest.scala
Comment thread core/src/test/scala/unit/kafka/log/ProducerStateManagerTest.scala Outdated
Comment thread storage/src/main/java/org/apache/kafka/server/log/internals/TxnMetadata.java Outdated
Comment thread storage/src/main/java/org/apache/kafka/server/log/internals/ProducerAppendInfo.java Outdated
Comment thread storage/src/main/java/org/apache/kafka/server/log/internals/ProducerStateEntry.java Outdated
@satishd satishd changed the title KAFKA-14558: Move LastRecord, TxnMetadata, BatchMetadata, ProducerStateEntry, and ProducerAppendInfo to the storage module KAFKA-14558: Move/Rewrite LastRecord, TxnMetadata, BatchMetadata, ProducerStateEntry, and ProducerAppendInfo to the storage module Jan 7, 2023
@satishd satishd force-pushed the mv-producer-statemanager-auxiliary-classes branch from 6e726dc to ef3b5cd Compare January 7, 2023 13:35
@satishd
Copy link
Copy Markdown
Member Author

satishd commented Jan 7, 2023

Thanks @ijuma for the review comments. Replied inline and/or addressed with the latest commits.

ijuma
ijuma reviewed Jan 7, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@ijuma ijuma left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. See comments below.

Comment thread storage/src/main/java/org/apache/kafka/server/log/internals/ProducerAppendInfo.java Outdated
Comment thread storage/src/main/java/org/apache/kafka/server/log/internals/ProducerStateEntry.java Outdated
Comment thread storage/src/main/java/org/apache/kafka/server/log/internals/ProducerStateEntry.java Outdated
Comment thread storage/src/main/java/org/apache/kafka/server/log/internals/ProducerAppendInfo.java Outdated
@satishd
Copy link
Copy Markdown
Member Author

satishd commented Jan 8, 2023

Thanks @ijuma for the review. Addressed them with the latest commit.

ijuma
ijuma approved these changes Jan 8, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@ijuma ijuma left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jan 8, 2023

JDK 8 build passed, the other builds had unrelated flaky failures.

@ijuma ijuma merged commit 2dec39d into apache:trunk Jan 8, 2023
@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jan 8, 2023 •
edited
Loading

@satishd I submitted a follow-up PR that removes the public mutable fields from ProducerStateEntry: #13091

rajinisivaram added a commit to confluentinc/kafka that referenced this pull request Jan 9, 2023
@rajinisivaram
Merge remote-tracking branch 'apache/trunk' into sync-upstream-trunk-…
6235e67 
…9-jan-2023

* apache/trunk: (16 commits)
  KAFKA-14570: Fix parenthesis in verifyFullFetchResponsePartitions output (apache#13072)
  MINOR: Remove public mutable fields from ProducerAppendInfo (apache#13091)
  KAFKA-14558: Move/Rewrite LastRecord, TxnMetadata, BatchMetadata, ProducerStateEntry, and ProducerAppendInfo to the storage module (apache#13043)
  KAFKA-14535: Fix flaky EndToEndAuthorization tests which were sensitive to ACL change reordering (apache#13086)
  KAFKA-9087 Replace log high watermark by future log high watermark wh… (apache#13075)
  MINOR: add error reason when controller failed to handle events (apache#13050)
  MINOR: doc: note how JDK-8136913 can affect client SASL (apache#13071)
  2023 (apache#13083)
  KAFKA-14279; Add 3.3.x to core compatibility tests (apache#13076)
  MINOR Fixed doc generation for LogConfig class in genTopicConfigDocs. (apache#13079)
  ...
guozhangwang pushed a commit to guozhangwang/kafka that referenced this pull request Jan 25, 2023
@satishd @guozhangwang
KAFKA-14558: Move/Rewrite LastRecord, TxnMetadata, BatchMetadata, Pro…
f73e6dc 
…ducerStateEntry, and ProducerAppendInfo to the storage module (apache#13043)

For broader context on this change, see:
* KAFKA-14470: Move log layer to storage module.

Reviewers: Ismael Juma <ismael@juma.me.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ijuma ijuma ijuma approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@satishd @ijuma