KAFKA-18207: Serde for handling transaction records#18136
KAFKA-18207: Serde for handling transaction records#18136mimaison merged 3 commits intoapache:trunkfrom
Conversation
github-actions
Bot
added
core
AndrewJSchofield
left a comment
AndrewJSchofield
left a comment
There was a problem hiding this comment.
Overall, looks good to me and achieves the overall aim nicely. Just one question though before I approve.
It seems that the dump log support for the transaction coordinator records was very rudimentary previously, and I think this PR will improve it just by using the JsonConverters instead of TransactionLog.formatRecordKeyAndValue. However, the values can be quite complicated these days and I wonder whether it's worth adding a complicated record to the tests also.
| @@ -144,32 +144,6 @@ object TransactionLog { | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
nit: I'd remove this blank line too.
| ).getMessage | ||
| ) | ||
|
|
||
| // A valid key and value should work. |
There was a problem hiding this comment.
I wonder whether we ought to have some testing of a more complicated value too. There are tagged fields and so on to consider.
There was a problem hiding this comment.
I've added a testcase with all fields set for TransactionValue. Is this what you had in mind?
There was a problem hiding this comment.
I think that will do. The point is to catch a situation where we inadvertently break this.
…e-old-protocol-versions * apache-github/trunk: KAFKA-18312: Added entityType: topicName to SubscribedTopicNames in ShareGroupHeartbeatRequest.json (apache#18285) HOTFIX: fix incompatible types: Optional<TimestampAndOffset> cannot be converted to Option<TimestampAndOffset> (apache#18284) MINOR Fix some test-catalog issues (apache#18272) KAFKA-18180: Move OffsetResultHolder to storage module (apache#18100) KAFKA-18301; Make coordinator records first class citizen (apache#18261) KAFKA-18262 Remove DefaultPartitioner and UniformStickyPartitioner (apache#18204) KAFKA-18296 Remove deprecated KafkaBasedLog constructor (apache#18257) KAFKA-12829: Remove old Processor and ProcessorSupplier interfaces (apache#18238) KAFKA-18292 Remove deprecated methods of UpdateFeaturesOptions (apache#18245) KAFKA-12829: Remove deprecated Topology#addProcessor of old Processor API (apache#18154) KAFKA-18035, KAFKA-18306, KAFKA-18092: Address TransactionsTest flaky tests (apache#18264) MINOR: change the default linger time in the new coordinator (apache#18274) KAFKA-18305: validate controller.listener.names is not in inter.broker.listener.name for kcontrollers (apache#18222) KAFKA-18207: Serde for handling transaction records (apache#18136) KAFKA-13722: Refactor Kafka Streams store interfaces (apache#18243) KAFKA-17131: Refactor TimeDefinitions (apache#18241) MINOR: Fix MessageFormatters (apache#18266) Mark flaky tests for Dec 18, 2024 (apache#18263)
Reviewers: Andrew Schofield <aschofield@confluent.io>
2 participants
This removes
transaction-coordinatoras a dependency ofstorage. This prevented makingtransaction-coordinatordepend oncoordinator-commonbecausecoordinator-commonitself depends onstoragethus making a loop.transaction-coordinatorwas only used instoragetests to get one constant.The new Serde,
TransactionCoordinatorRecordSerde, mimicsGroupCoordinatorRecordSerdeand is also used inDumpLogSegmentsinstead ofTransactionLog. Following this PR, we will now be able to moveDumpLogSegmentstotoolsasTransactionLogwas the last class it depended onto fromcore.We should be able to refactor
TransactionLogto useTransactionCoordinatorRecordSerde, or even completely replace it. But let's do that in follow up PRs.Committer Checklist (excluded from commit message)