Skip to content

MINOR: fix streams test-utils dependencies#4821

Merged
guozhangwang merged 1 commit into
apache:trunkfrom
vvcephei:fix-streams-test-utils-deps
Apr 5, 2018
Merged

MINOR: fix streams test-utils dependencies#4821
guozhangwang merged 1 commit into
apache:trunkfrom
vvcephei:fix-streams-test-utils-deps

Conversation

@vvcephei

@vvcephei vvcephei commented Apr 3, 2018

Copy link
Copy Markdown
Contributor

Follow-up to @ewencp 's PR #4816 (a fix of my #4760).

Break the cycle on the other side so that :streams:test-utils can declare a regular dependency on :streams.

To test that we don't re-introduce the bug that #4816 fixed, do the following:

./gradlew clean installAll releaseTarGzAll
tar --list -f ./core/build/distributions/kafka_2.11-1.2.0-SNAPSHOT.tgz | grep 'class'

That latter command should return only the following (instead of 580 class files):

kafka_2.11-1.2.0-SNAPSHOT/bin/kafka-run-class.sh
kafka_2.11-1.2.0-SNAPSHOT/bin/windows/kafka-run-class.bat

Committer Checklist (excluded from commit message)

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

@vvcephei

vvcephei commented Apr 3, 2018

Copy link
Copy Markdown
Contributor Author

@mjsax @guozhangwang @bbejeck @ewencp

Hey all, do you mind giving this a review? I also sent a message to the mailing list.

@guozhangwang

Copy link
Copy Markdown
Contributor

I like this PR since it does not users to declare two dependencies, streams and streams-utils, but just the streams-utils.

@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, @vvcephei. I tried it out works as expected.

LGTM

@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 following up on this @vvcephei !

@vvcephei

vvcephei commented Apr 5, 2018

Copy link
Copy Markdown
Contributor Author

Thanks folks. If there's no more feedback, should we merge this?

@guozhangwang guozhangwang merged commit d5db4e9 into apache:trunk Apr 5, 2018
@vvcephei vvcephei deleted the fix-streams-test-utils-deps branch April 5, 2018 14:42
@mjsax mjsax added the streams label May 7, 2018
ying-zheng pushed a commit to ying-zheng/kafka that referenced this pull request Jul 6, 2018
Reviewers: Bill Bejeck <bill@confluent.io>, Matthias J. Sax <matthias@confluent.io>, Guozhang Wang <wangguoz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants