KAFKA-10864: Convert end txn marker schema to use auto-generated protocol by dengziming · Pull Request #9766 · apache/kafka

dengziming · 2020-12-18T06:36:04Z

More detailed description of your change

Convert end txn marker schema to use auto-generated protocol EndTxnMarker
substitute CURRENT_END_TXN_MARKER_VALUE_SIZE with an endTnxMarkerValueSize method since the size is accumulated from EndTxnMarker.
add buffer to EndTransactionMarker to avoid twice compute from serializeValue and endTnxMarkerValueSize.
flexibleVersions is set to none.

Summary of testing strategy (including rationale)
A unit test to verify serialize and deserialize

Committer Checklist (excluded from commit message)

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

abbccdda

Thanks for the PR, one high level question I have is that whether the new auto generated protocol format is exactly the same as old hard-coded schema format, since this field is used inter-broker. Could you write a test to prove that they could translate between each other?

abbccdda · 2020-12-18T17:38:14Z

Add a comment for the field. Also do you think we need flexible version support here?

Firstly I tried to use flexible version here, but it's not the same as the old hard-coded schema format. Could we bump the version and support flexible version from version 1 ?

dengziming · 2020-12-19T03:51:42Z

Thanks for the PR, one high level question I have is that whether the new auto generated protocol format is exactly the same as old hard-coded schema format, since this field is used inter-broker. Could you write a test to prove that they could translate between each other?

Thank you, I tested that the 2 schemas could translate between each other locally, now I add the code to EndTransactionMarkerTest.

dengziming · 2021-01-18T13:00:32Z

ping @abbccdda , Also ping @chia7712 to have a look.

github-actions · 2024-12-01T03:46:16Z

This PR is being marked as stale since it has not had any activity in 90 days. If you
would like to keep this PR alive, please leave a comment asking for a review. If the PR has
merge conflicts, update it with the latest from the base branch.

If you are having difficulty finding a reviewer, please reach out on the [mailing list](https://kafka.apache.org/contact).

If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed.

chia7712 · 2024-12-01T11:41:10Z

@dengziming could you please fix the conflicts?

dengziming · 2024-12-05T13:21:29Z

Hi, @chia7712 , Thanks for noticing this PR, I have rebased on trunk and the test looks fine.

We have a controversy here that in the past the length of EndTransactionMarker is hard-coded to 6, however it is dynamically acquired from it's serialized ByteBuffer, so I stored buffer of a EndTransactionMarker when it's created, this will add some state but reduce one calculation.

Another alternative is to change MemoryRecords.writeEndTransactionalMarker signature to directly receive (xxx..., ByteBuffer markerBuffer, ControlRecordType markerType) as the below screenshot, but this involves more refactor, WDYT.

chia7712

@dengziming sorry for delay review :(

one small comment is left. PTAL

chia7712

@dengziming thanks for this patch!

chia7712 · 2025-03-01T14:26:04Z

 public class EndTransactionMarkerTest {

+    // Old hard-coded schema, used to validate old hard-coded schema format is exactly the same as new auto generated protocol format
+    private Schema v0Schema = new Schema(


could you please add final?

chia7712 · 2025-03-01T14:33:37Z


-        int coordinatorEpoch = value.getInt(2);
-        return new EndTransactionMarker(type, coordinatorEpoch);
+    public int endTxnMarkerValueSize() {


Could you please add unit test for this method?

Yes, it's good to test it since it's public.

chia7712

@dengziming thanks for this patch, overall LGTM and one last comment is left

chia7712 · 2025-03-04T11:10:12Z

            log.debug("Received end transaction marker value version {}. Parsing as version {}", version,
-                    CURRENT_END_TXN_MARKER_VERSION);
+                    EndTxnMarker.HIGHEST_SUPPORTED_VERSION);
+        EndTxnMarker marker = new EndTxnMarker(new ByteBufferAccessor(value), version);


According to the debug message, we should use EndTxnMarker.HIGHEST_SUPPORTED_VERSION instead of version to parse it as EndTxnMarker.HIGHEST_SUPPORTED_VERSION, right?

Oh, you are so careful about this, though it seems obscure, I agree with you that we should indeed use EndTxnMarker.HIGHEST_SUPPORTED_VERSION.

abbccdda reviewed Dec 18, 2020

View reviewed changes

dengziming requested a review from abbccdda January 5, 2021 10:36

dengziming force-pushed the end-txn-auto-protocol branch from 62c0038 to fe6b884 Compare January 18, 2021 12:59

dengziming mentioned this pull request Jan 25, 2021

MINOR: Remove toStruct and fromStruct methods from generated protocol classes #9960

Merged

3 tasks

dengziming force-pushed the end-txn-auto-protocol branch from fe6b884 to a0129ac Compare January 31, 2021 09:25

dengziming force-pushed the end-txn-auto-protocol branch from a0129ac to 63d420a Compare February 28, 2021 04:37

dengziming force-pushed the end-txn-auto-protocol branch from 63d420a to 340cabc Compare March 11, 2021 07:46

dengziming force-pushed the end-txn-auto-protocol branch from 340cabc to 77891e9 Compare May 19, 2021 10:49

github-actions Bot added the stale Stale PRs label Dec 1, 2024

dengziming force-pushed the end-txn-auto-protocol branch from 77891e9 to 00e9aae Compare December 5, 2024 12:51

github-actions Bot added the clients label Dec 5, 2024

github-actions Bot removed the stale Stale PRs label Dec 23, 2024

chia7712 reviewed Feb 26, 2025

View reviewed changes

Comment thread clients/src/main/java/org/apache/kafka/common/record/EndTransactionMarker.java

dengziming and others added 2 commits February 27, 2025 16:38

end txn marker auto-generated

5fcc632

add version check back

be75709

dengziming force-pushed the end-txn-auto-protocol branch from 00e9aae to be75709 Compare February 27, 2025 09:23

fix import order

c5136ce

chia7712 reviewed Mar 1, 2025

View reviewed changes

add test for new method

fec6d59

chia7712 reviewed Mar 4, 2025

View reviewed changes

Use highest_version when version exceed highest_version

209410a

chia7712 approved these changes Mar 5, 2025

View reviewed changes

chia7712 merged commit 1bfa4cd into apache:trunk Mar 5, 2025

Conversation

dengziming commented Dec 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

abbccdda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dengziming commented Dec 19, 2020

Uh oh!

dengziming commented Jan 18, 2021

Uh oh!

github-actions Bot commented Dec 1, 2024

Uh oh!

chia7712 commented Dec 1, 2024

Uh oh!

dengziming commented Dec 5, 2024

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dengziming commented Dec 18, 2020 •

edited

Loading