KAFKA-5370: Replace uses of the old consumer with the new consumer when possible by vahidhashemian · Pull Request #3320 · apache/kafka

vahidhashemian · 2017-06-13T18:46:17Z

Also, methods in ClientUtils that are called by server or tools code are introduced in AdminUtils with the implementation living in AdminUtils. All the existing callers apart from the old clients call the AdminUtils methods.

asfbot · 2017-06-13T20:19:59Z

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

asfbot · 2017-06-13T20:45:09Z

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

vahidhashemian · 2017-06-13T20:50:18Z

@ijuma Is this close to what you had in mind by your earlier comment? Thanks.

asfgit · 2017-06-21T21:58:10Z

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

asfgit · 2017-06-21T22:04:38Z

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

asfgit · 2017-06-21T23:09:01Z

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

asfgit · 2017-06-22T01:10:21Z

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

ijuma

Thanks for the PR and sorry for the delay. I left some comments. Also, it would be great if we could update ProducerBounceTest to use the new consumer.

ijuma · 2017-08-04T13:03:44Z

We should use a JUnit assertion (i.e. assertTrue).

ijuma · 2017-08-04T13:06:11Z

Can you elaborate on why many of these tests were removed?

In this PR I used an API that did not provide some of the details the deprecated API provided. In the new patch I switch to Metadata API that seem to cover the missing info. So, these tests will be back in the new patch.

ijuma · 2017-08-04T13:11:31Z

Maybe this should be a method in BrokerEndPoint.

ijuma · 2017-08-04T13:12:28Z

This doesn't seem to make sense since we are now using the new consumer.

In the new patch this method is no longer needed.

ijuma · 2017-08-04T13:13:35Z

On second thought, this should probably not be here. AdminUtils is the place for methods that interact with ZK. We could add it to the ClientUtils in the client module maybe (that makes it clear that we are communicating with Kafka via the protocol).

Do we want to deprecate ClientUtils? If so, should we find another host for this method? Suggestions? What do you think about channelToAnyBroker and channelToOffsetManager?

ijuma · 2017-08-04T13:16:51Z

One more thing: we should consider removing methods from the Scala ClientUtils if the new implementation is suitable across the board (like the method that parses the broker list).

asfgit · 2017-08-04T14:40:08Z

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

asfgit · 2017-08-04T14:43:53Z

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

vahidhashemian

@ijuma Thanks for reviewing the PR. I tried to address your comments. I asked inline about where to put a couple of existing methods. Let me know if you have any suggestions. Thanks.

vahidhashemian · 2017-08-07T19:33:14Z

Do we want to deprecate ClientUtils? If so, should we find another host for this method? Suggestions? What do you think about channelToAnyBroker and channelToOffsetManager?

vahidhashemian · 2017-08-07T19:36:35Z

In the new patch this method is no longer needed.

vahidhashemian · 2017-08-07T21:00:31Z

In this PR I used an API that did not provide some of the details the deprecated API provided. In the new patch I switch to Metadata API that seem to cover the missing info. So, these tests will be back in the new patch.

asfgit · 2017-08-08T06:06:07Z

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

asfgit · 2017-08-08T06:26:39Z

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

vahidhashemian · 2017-08-08T18:33:44Z

@ijuma I'm making some additional adjustments. Please hold off reviewing until the next patch is submitted. Thanks.

asfgit · 2017-08-09T00:48:43Z

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

asfgit · 2017-08-09T01:14:42Z

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

asfgit · 2017-08-09T20:08:11Z

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

asfgit · 2017-08-09T21:05:49Z

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

vahidhashemian · 2017-08-10T15:39:06Z

@ijuma The PR should be in a better shape now. We may need to move the methods from ClientUtils, as you suggested. If so, please let me now which file(s) you think should host them.

…ssible - Uses of the old consumers in tools and tests where the new consumer would work as well (or better).

…ssible Uses of the old consumers in tools and tests where the new consumer would work as well (or better) should be updated. Conflicts: core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala

…ssible Uses of the old consumers in tools and tests where the new consumer would work as well (or better) should be updated. Conflicts: core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala Conflicts: core/src/test/scala/integration/kafka/api/ProducerBounceTest.scala core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala

ijuma · 2018-06-14T00:50:38Z

Sorry, I dropped the ball on this review. I don't think this PR is relevant anymore, but I did include you as a co-author in #5220 which overlaps a bit with one of the changes in this PR.

This does the minimal amount of work so that the tool relies on public non-deprecated APIs (i.e. so that it doesn't rely on old clients code). Additional improvements have been proposed via KIP-308. There are a few other PRs that touch this class with overlapping goals: - apache#2891 - apache#3051 - apache#3320 One of them remains relevant in the context of KIP-308, but the others are not. I included the authors of the 3 PRs as co-authors. Co-authored-by: Arseniy Tashoyan <tashoyan@gmail.com> Co-authored-by: Vahid Hashemian <vahidhashemian@us.ibm.com> Co-authored-by: Mohammed Amine GARMES Co-authored-by: Ismael Juma <ismael@juma.me.uk>

This does the minimal amount of work so that the tool relies on public non-deprecated APIs (i.e. it no longer relies on Scala clients code). Additional improvements (not included here) have been proposed via KIP-308. There are a few other PRs that touch this class with overlapping goals: - #2891 - #3051 - #3320 One of them remains relevant in the context of KIP-308, but the others have been superseded. I included the authors of the 3 PRs as co-authors. Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com>, Vahid Hashemian <vahidhashemian@us.ibm.com>, Manikumar Reddy <manikumar.reddy@gmail.com> Co-authored-by: Arseniy Tashoyan <tashoyan@gmail.com> Co-authored-by: Vahid Hashemian <vahidhashemian@us.ibm.com> Co-authored-by: Mohammed Amine GARMES Co-authored-by: Ismael Juma <ismael@juma.me.uk>

This does the minimal amount of work so that the tool relies on public non-deprecated APIs (i.e. it no longer relies on Scala clients code). Additional improvements (not included here) have been proposed via KIP-308. There are a few other PRs that touch this class with overlapping goals: - apache#2891 - apache#3051 - apache#3320 One of them remains relevant in the context of KIP-308, but the others have been superseded. I included the authors of the 3 PRs as co-authors. Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com>, Vahid Hashemian <vahidhashemian@us.ibm.com>, Manikumar Reddy <manikumar.reddy@gmail.com> Co-authored-by: Arseniy Tashoyan <tashoyan@gmail.com> Co-authored-by: Vahid Hashemian <vahidhashemian@us.ibm.com> Co-authored-by: Mohammed Amine GARMES Co-authored-by: Ismael Juma <ismael@juma.me.uk>

vahidhashemian force-pushed the KAFKA-5370 branch 4 times, most recently from b7e2211 to 96f91b7 Compare June 21, 2017 20:54

ijuma reviewed Aug 4, 2017

View reviewed changes

vahidhashemian commented Aug 8, 2017

View reviewed changes

vahidhashemian force-pushed the KAFKA-5370 branch from 96f91b7 to 1303425 Compare August 8, 2017 05:05

vahidhashemian changed the title ~~KAFKA-5370 (WIP): Replace uses of the old consumer with the new consumer when possible~~ KAFKA-5370: Replace uses of the old consumer with the new consumer when possible Aug 8, 2017

vahidhashemian force-pushed the KAFKA-5370 branch from 9263bb6 to 23ffb1b Compare August 9, 2017 20:07

vahidhashemian added 3 commits September 21, 2017 17:58

KAFKA-5370: Replace use of old consumer with the new consumer when po…

4abf8a4

…ssible - Uses of the old consumers in tools and tests where the new consumer would work as well (or better).

KAFKA-5370: Replace use of old consumer with the new consumer when po…

2dc720b

…ssible Uses of the old consumers in tools and tests where the new consumer would work as well (or better) should be updated. Conflicts: core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala

Additional cleanup

ca06529

vahidhashemian force-pushed the KAFKA-5370 branch from 23ffb1b to 2c282a9 Compare September 22, 2017 19:08

vahidhashemian force-pushed the KAFKA-5370 branch from 2c282a9 to d26e488 Compare September 22, 2017 19:10

ijuma mentioned this pull request Jun 14, 2018

MINOR: Use KafkaConsumer in GetOffsetShell #5220

Merged

3 tasks

ijuma closed this Jun 14, 2018

Conversation

vahidhashemian commented Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asfbot commented Jun 13, 2017

Uh oh!

asfbot commented Jun 13, 2017

Uh oh!

vahidhashemian commented Jun 13, 2017

Uh oh!

asfgit commented Jun 21, 2017

Uh oh!

asfgit commented Jun 21, 2017

Uh oh!

asfgit commented Jun 21, 2017

Uh oh!

asfgit commented Jun 22, 2017

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Aug 4, 2017

Uh oh!

asfgit commented Aug 4, 2017

Uh oh!

asfgit commented Aug 4, 2017

Uh oh!

vahidhashemian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfgit commented Aug 8, 2017

Uh oh!

asfgit commented Aug 8, 2017

Uh oh!

vahidhashemian commented Aug 8, 2017

Uh oh!

asfgit commented Aug 9, 2017

Uh oh!

asfgit commented Aug 9, 2017

Uh oh!

asfgit commented Aug 9, 2017

Uh oh!

asfgit commented Aug 9, 2017

Uh oh!

vahidhashemian commented Aug 10, 2017

Uh oh!

ijuma commented Jun 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vahidhashemian commented Jun 13, 2017 •

edited

Loading