Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

KAFKA-8618: Replace Txn marker with automated protocol #7039

Merged
merged 2 commits into from Mar 19, 2020
Merged

KAFKA-8618: Replace Txn marker with automated protocol #7039

merged 2 commits into from Mar 19, 2020

Conversation

abbccdda
Copy link
Contributor

@abbccdda abbccdda commented Jul 7, 2019

As title. This one is tricky because the original txn marker entry has been widely used, so we choose to minimize the change by making the transformation internal for the txn marker class.

Committer Checklist (excluded from commit message)

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

@abbccdda abbccdda force-pushed the txn_marker branch 3 times, most recently from b629f53 to 7ab4850 Compare January 10, 2020 17:14
@hachikuji
Copy link
Contributor

retest this please

1 similar comment
@hachikuji
Copy link
Contributor

retest this please

@abbccdda
Copy link
Contributor Author

Checked both test failures are flaky

@hachikuji
Copy link
Contributor

retest this please

1 similar comment
@abbccdda
Copy link
Contributor Author

abbccdda commented Feb 6, 2020

retest this please

@hachikuji
Copy link
Contributor

ok to test

@guozhangwang
Copy link
Contributor

retest this please

@guozhangwang
Copy link
Contributor

test this please

@abbccdda
Copy link
Contributor Author

abbccdda commented Feb 11, 2020 •
edited

Got 1/2 green build https://builds.apache.org/job/kafka-pr-jdk11-scala2.13/4641/
The two failure test cases for the other build are flaky ones:
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/628/

@abbccdda
Copy link
Contributor Author

@mimaison Hey Mickael, could you take a look of this PR?

mimaison
mimaison requested changes Mar 13, 2020
View reviewed changes
Copy link
Member

@mimaison mimaison left a comment

Choose a reason for hiding this comment

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

Thanks @abbccdda
I've made a quick pass and left a few small comments

clients/src/main/java/org/apache/kafka/common/requests/WriteTxnMarkersResponse.java Outdated Show resolved Hide resolved
clients/src/test/java/org/apache/kafka/common/requests/WriteTxnMarkersRequestTest.java Outdated Show resolved Hide resolved
clients/src/test/java/org/apache/kafka/common/requests/WriteTxnMarkersResponseTest.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/common/requests/WriteTxnMarkersRequest.java Outdated Show resolved Hide resolved
@abbccdda
Copy link
Contributor Author

@mimaison Thanks for the review, addressed.

@mimaison
Copy link
Member

retest this please

1 similar comment
@mimaison
Copy link
Member

retest this please

@abbccdda
Copy link
Contributor Author

@mimaison Could you help trigger another test?

@mimaison
Copy link
Member

retest this please

@abbccdda
Copy link
Contributor Author

@mimaison Looks like the Jenkins jobs are killed, mind kicking off another one?

@mimaison
Copy link
Member

retest this please

@abbccdda
Copy link
Contributor Author

@mimaison Do you mind triggering another test? The PR was rebased to include two broken unit test changes landed yesterday.

@guozhangwang
Copy link
Contributor

test this please

2 similar comments
@guozhangwang
Copy link
Contributor

test this please

@guozhangwang
Copy link
Contributor

test this please

@abbccdda
Copy link
Contributor Author

Looks like the test was flaky and not related: org.apache.kafka.streams.processor.internals.StateDirectoryTest.shouldReturnEmptyArrayIfListFilesReturnsNull
Which is actively being fixed: #8310

@mimaison Let me know if you think we should kick off another test just for sanity

@mimaison
Copy link
Member

Thanks for thr updates @abbccdda. I plan to take another look this week

@abbccdda
Copy link
Contributor Author

Rebased

@mimaison
Copy link
Member

retest this please

@mimaison mimaison merged commit c7164a3 into apache:trunk Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mimaison mimaison mimaison approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@abbccdda @hachikuji @guozhangwang @mimaison