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.

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

KAFKA-5427: Transactional producer should allow FindCoordinator in error state #3297

Closed
wants to merge 1 commit into from

Conversation

hachikuji
Copy link
Contributor

No description provided.

@asfbot
Copy link

asfbot commented Jun 11, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/5149/
Test PASSed (JDK 7 and Scala 2.11).

@asfbot
Copy link

asfbot commented Jun 11, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/5133/
Test PASSed (JDK 8 and Scala 2.12).

@@ -1279,16 +1345,43 @@ public void shouldNotSendAbortTxnRequestWhenOnlyAddOffsetsRequestFailed() throws

TransactionalRequestResult abortResult = transactionManager.beginAbortingTransaction();

prepareAddOffsetsToTxnResponse(Errors.TOPIC_AUTHORIZATION_FAILED, consumerGroupId, pid, epoch);
prepareAddOffsetsToTxnResponse(Errors.GROUP_AUTHORIZATION_FAILED, consumerGroupId, pid, epoch);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this changed?

Copy link
Contributor Author

@hachikuji hachikuji Jun 12, 2017

Choose a reason for hiding this comment

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

The previous error is not expected for AddOffsets, which means it is a fatal error. The intent of the test case was apparently to trigger an abortable error. Note that I've added a test case for the fatal error case below.

Copy link
Contributor

@apurvam apurvam left a comment

Choose a reason for hiding this comment

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

LGTM, barring a minor comment.


// we do not transition to ABORTABLE_ERROR since we were already aborting
assertTrue(transactionManager.isAborting());
assertFalse(transactionManager.hasError());
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps better to assert that transactionManager.hasAbortableError is false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The hasError check is strictly stronger. It covers both hasAbortableError and hasFatalError.

Copy link
Contributor

@guozhangwang guozhangwang left a comment

Choose a reason for hiding this comment

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

LGTM

@hachikuji
Copy link
Contributor Author

Merging to trunk and 0.11.0

asfgit pushed a commit that referenced this pull request Jun 12, 2017
…ror state

Author: Jason Gustafson <jason@confluent.io>

Reviewers: Ismael Juma <ismael@juma.me.uk>, Apurva Mehta <apurva@confluent.io>, Guozhang Wang <wangguoz@gmail.com>

Closes #3297 from hachikuji/KAFKA-5427

(cherry picked from commit 43e935a)
Signed-off-by: Jason Gustafson <jason@confluent.io>
@asfgit asfgit closed this in 43e935a Jun 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants