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-5171 : TC should not accept empty string transactional id #2973

Closed
wants to merge 1 commit into from

Conversation

umesh9794
Copy link
Contributor

This is an initial PR. Changed the unit tests accordingly as per the expectation from TC.

@umesh9794 umesh9794 changed the title Kafka 5171:TC should not accept empty string transactional id KAFKA-5171 : TC should not accept empty string transactional id May 4, 2017
@asfbot
Copy link

asfbot commented May 4, 2017

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

@asfbot
Copy link

asfbot commented May 4, 2017

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

@asfbot
Copy link

asfbot commented May 4, 2017

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

@asfbot
Copy link

asfbot commented May 5, 2017

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

@asfbot
Copy link

asfbot commented May 5, 2017

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

@asfbot
Copy link

asfbot commented May 5, 2017

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

@umesh9794
Copy link
Contributor Author

@guozhangwang please review this PR and let me know if that serves the purpose. Can we also refactor TransactionCoordinator.handleInitPid() to leverage the existing validateTransactionalId() method?

@guozhangwang
Copy link
Contributor

@umesh9794 thank you for the PR. I think the purpose is to accept null transactionalId but not empty string, while your PR changes both? Also there are a couple of PRs ongoing that may change TransactionalCoordinator class as well, so your PR may need to be rebased after those commits, just a heads up :)

@asfbot
Copy link

asfbot commented May 9, 2017

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

@umesh9794
Copy link
Contributor Author

Ah..sorry I missed that null should be accepted, thanks for the review @guozhangwang. I have updated the PR. Sure will wait to rebase this PR after those commits and thanks again for telling this in advance :)

@asfbot
Copy link

asfbot commented May 9, 2017

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

@asfbot
Copy link

asfbot commented May 9, 2017

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

KAFKA-5171 : TC should not accept empty string transactional id
@umesh9794
Copy link
Contributor Author

@guozhangwang should I rebase it now or some more changes are about to be pushed?

@asfbot
Copy link

asfbot commented May 17, 2017

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

@asfbot
Copy link

asfbot commented May 17, 2017

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

@guozhangwang
Copy link
Contributor

@guozhangwang should I rebase it now or some more changes are about to be pushed?

The major changes have been pushed, feel free to rebase now.

@umesh9794
Copy link
Contributor Author

@guozhangwang . I rebased the changes and created a new PR because I got messed with many conflicts in this PR. Please review that updated PR and let me know your comments.

asfgit pushed a commit that referenced this pull request May 18, 2017
This is a rebase version of [PR#2973](#2973).

guozhangwang , please review this updated PR.

Author: umesh chaudhary <umesh9794@gmail.com>

Reviewers: Guozhang Wang <wangguoz@gmail.com>, Jason Gustafson <jason@confluent.io>

Closes #3086 from umesh9794/mylocal
@umesh9794 umesh9794 closed this May 27, 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
Development

Successfully merging this pull request may close these issues.

3 participants