Skip to content

MINOR: Fix the missing ApiUtils tests in streams#6003

Merged
guozhangwang merged 2 commits into
apache:trunkfrom
mrsrinivas:apiutil-ut
Dec 14, 2018
Merged

MINOR: Fix the missing ApiUtils tests in streams#6003
guozhangwang merged 2 commits into
apache:trunkfrom
mrsrinivas:apiutil-ut

Conversation

@mrsrinivas

@mrsrinivas mrsrinivas commented Dec 5, 2018

Copy link
Copy Markdown
Contributor

Committer Checklist (excluded from commit message)

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

@guozhangwang

guozhangwang commented Dec 6, 2018

Copy link
Copy Markdown
Contributor

@bbejeck could you take a look?

bbejeck
bbejeck reviewed Dec 6, 2018
View reviewed changes

@bbejeck bbejeck left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the contribution @mrsrinivas. Just one minor nit otherwise it looks good.

Comment thread streams/src/main/java/org/apache/kafka/streams/internals/ApiUtils.java Outdated
bbejeck
bbejeck reviewed Dec 6, 2018
View reviewed changes

@bbejeck bbejeck left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the follow-up @mrsrinivas. Just one minor nit, otherwise looks good.

bbejeck
bbejeck reviewed Dec 6, 2018
View reviewed changes

@bbejeck bbejeck left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the follow-up @mrsrinivas. Just one minor nit, otherwise this looks good.

@mrsrinivas

mrsrinivas commented Dec 6, 2018

Copy link
Copy Markdown
Contributor Author

@guozhangwang @bbejeck, thanks for the review comment. I addressed it now.

@bbejeck

bbejeck commented Dec 6, 2018

Copy link
Copy Markdown
Member

My apologies for the multiple comments, GitHub was acting wonky this morning for me, and I got a 503 error, so I resubmitted.

@mrsrinivas

mrsrinivas commented Dec 7, 2018

Copy link
Copy Markdown
Contributor Author

May be that API follows acks=0 :)

bbejeck
bbejeck approved these changes Dec 7, 2018
View reviewed changes

@bbejeck bbejeck left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@mrsrinivas thanks for the contribution. LGTM

@mrsrinivas

mrsrinivas commented Dec 7, 2018

Copy link
Copy Markdown
Contributor Author

@bbejeck thank you.

ping @guozhangwang @mjsax for final review.

mjsax
mjsax reviewed Dec 7, 2018
View reviewed changes

@mjsax mjsax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the PR!

Couple of nit suggestions about code style.

Comment thread streams/src/test/java/org/apache/kafka/streams/internals/ApiUtilsTest.java Outdated
Comment thread streams/src/test/java/org/apache/kafka/streams/internals/ApiUtilsTest.java Outdated
Comment thread streams/src/test/java/org/apache/kafka/streams/internals/ApiUtilsTest.java Outdated
Comment thread streams/src/test/java/org/apache/kafka/streams/internals/ApiUtilsTest.java Outdated
Comment thread streams/src/test/java/org/apache/kafka/streams/internals/ApiUtilsTest.java Outdated
Comment thread streams/src/test/java/org/apache/kafka/streams/internals/ApiUtilsTest.java Outdated
Comment thread streams/src/test/java/org/apache/kafka/streams/internals/ApiUtilsTest.java Outdated
@mrsrinivas

mrsrinivas commented Dec 8, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thank you very much @mjsax, for constructive review comments.
I pushed the code changes. TIA

@mrsrinivas

mrsrinivas commented Dec 11, 2018

Copy link
Copy Markdown
Contributor Author

ping @mjsax @guozhangwang for review.

@mrsrinivas

mrsrinivas commented Dec 12, 2018

Copy link
Copy Markdown
Contributor Author

@mjsax, I resolved merge conflicts after PR 5954 merge to trunk.

It was nice code cleanup 👍

guozhangwang
guozhangwang approved these changes Dec 14, 2018
View reviewed changes

@guozhangwang guozhangwang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM！

@guozhangwang
guozhangwang merged commit 88443b4 into apache:trunk Dec 14, 2018
@mrsrinivas
mrsrinivas deleted the apiutil-ut branch December 14, 2018 03:47
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
@mrsrinivas @pengxiaolong
Fix the missing ApiUtils tests in streams module. (apache#6003)
dcdbdfe 
Reviewers: Matthias J. Sax <matthias@confluent.io>, Guozhang Wang <guozhang@confluent.io>, Bill Bejeck <bill@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mjsax mjsax mjsax left review comments
@bbejeck bbejeck bbejeck approved these changes
@guozhangwang guozhangwang guozhangwang approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mrsrinivas @guozhangwang @bbejeck @mjsax