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-4716: Send request to controller #2522

Closed
wants to merge 5 commits into from
Closed

KAFKA-4716: Send request to controller #2522

wants to merge 5 commits into from

Conversation

enothereska
Copy link
Contributor

@enothereska enothereska commented Feb 9, 2017

This PR fixes a blocker issue, where the streams client code cannot talk to the controller. It also enables a system test that was previously failing.

This PR is for trunk only. A separate PR with just the fix (but not the tests) will be created for 0.10.2.

@enothereska
Copy link
Contributor Author

[WiP] because system test has yet to run.

Copy link
Contributor

@dguy dguy left a comment

Choose a reason for hiding this comment

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

LGTM (assuming the system tests pass) - should we also make the streams_smoke_test run with 3 brokers?

One side note (not your problem), the StreamsKafkaClient doesn't have any tests. We need to rectify this

@enothereska
Copy link
Contributor Author

Ok, I'll test the smoke test with 3 brokers first. I agree on the unit tests.

@asfbot
Copy link

asfbot commented Feb 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/1588/
Test PASSed (JDK 8 and Scala 2.11).

@asfbot
Copy link

asfbot commented Feb 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/1585/
Test PASSed (JDK 8 and Scala 2.12).

@enothereska enothereska changed the title KAFKA-4716: Send request to controller [WiP] KAFKA-4716: Send request to controller Feb 9, 2017
@enothereska
Copy link
Contributor Author

Tests at scale pass for me on AWS. However, there appears to be a bug in the benchmark when I pass parameter "all" instead of each test. I'll open a separate JIRA for that, it is not related to this blocker.

@asfbot
Copy link

asfbot commented Feb 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/1585/
Test FAILed (JDK 7 and Scala 2.10).

@asfbot
Copy link

asfbot commented Feb 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/1589/
Test PASSed (JDK 8 and Scala 2.12).

@asfbot
Copy link

asfbot commented Feb 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/1592/
Test PASSed (JDK 8 and Scala 2.11).

@@ -35,7 +35,7 @@ def __init__(self, test_context):


@cluster(num_nodes=9)
@matrix(test=["all"], scale=[1])
@matrix(test=["produce", "consume", "count", "processstream", "processstreamwithsink", "processstreamwithstatestore", "processstreamwithcachedstatestore", "kstreamktablejoin", "kstreamkstreamjoin", "ktablektablejoin"], scale=[1, 2, 3])
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"all" was meant to be a convenience function when running locally, e.g., in a debugger. Not at scale. The reason is that a bit subtle. If I want to see how something like count scales, I don't want to mix the count run on one instance, with a random function that could still be running on another, e.g., consume. So all instances must be running the same function for the scale test to make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a comment on top of class.

@asfbot
Copy link

asfbot commented Feb 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/1589/
Test PASSed (JDK 7 and Scala 2.10).

@asfbot
Copy link

asfbot commented Feb 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/1595/
Test PASSed (JDK 8 and Scala 2.11).

@asfbot
Copy link

asfbot commented Feb 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/1592/
Test PASSed (JDK 8 and Scala 2.12).

@asfbot
Copy link

asfbot commented Feb 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/1592/
Test PASSed (JDK 7 and Scala 2.10).

@enothereska
Copy link
Contributor Author

Copy link
Contributor

@dguy dguy left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Merged to trunk.

@asfgit asfgit closed this in b865a8b Feb 9, 2017
@enothereska enothereska deleted the KAFKA-4716-metadata branch February 10, 2017 07:43
hachikuji pushed a commit to confluentinc/kafka that referenced this pull request Feb 23, 2017
This PR fixes a blocker issue, where the streams client code cannot talk to the controller. It also enables a system test that was previously failing.

This PR is for trunk only. A separate PR with just the fix (but not the tests) will be created for 0.10.2.

Author: Eno Thereska <eno@confluent.io>
Author: Eno Thereska <eno.thereska@gmail.com>

Reviewers: Damian Guy, Ismael Juma, Matthias J. Sax, Guozhang Wang

Closes apache#2522 from enothereska/KAFKA-4716-metadata
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.

6 participants