KAFKA-6123: Give client MetricsReporter auto-generated client.id by KevinLiLu · Pull Request #5383 · apache/kafka

KevinLiLu · 2018-07-18T01:37:16Z

The client's MetricsReporter class currently receives the client.id field only if the user has configured it. The auto-generated client.id is not passed to the MetricsReporter.

Give auto generated client.id as a config override to MetricsReporter (same way KAFKA-4756 handles it)

Committer Checklist (excluded from commit message)

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

KevinLiLu · 2018-07-18T17:35:05Z

Hi @cmccabe @ijuma @junrao @guozhangwang ,

Can I get a review?

Thanks!

cmccabe · 2018-07-18T23:08:09Z

Hmm, does this require a KIP? I'm not sure...

KevinLiLu · 2018-07-18T23:20:47Z

@cmccabe I wasn't sure about that either... This PR is to address an edge case where the client.id does not get passed to the MetricsReporter, as opposed to giving a completely new configuration.

ijuma · 2018-07-20T17:14:07Z

This seems like a big fix to me, not sure a KIP is needed.

cmccabe · 2018-07-31T00:12:48Z

Can you check that the auto-generated client ID appears here?

It is loosely checking the auto-generated client ID by checking for the prefix producer-. This is because there are many other tests that create clients without specifying the client.id, which increments the static CONSUMER_CLIENT_ID_SEQUENCE and PRODUCER_CLIENT_ID_SEQUENCE. This is problematic as the auto-generated client ID value then depends on the order of test execution.

There are no getters/setters for this client ID sequence so it is not easy to test for exact auto-generated client ID.

I see two options here:

(1) Make all other tests set a client ID
We could make all other tests set the client.id field so that the auto-generated client ID will be suffixed with -1 (producer-1, consumer-1). All future tests would have to follow this and set client ids to ensure these tests always pass (if tests are not always executed in the same order, then we could get into a bad situation where it fails sometimes).

(2) Create getter methods to mock
We could create getter methods getConsumerClientIdSequence() and getProducerClientIdSequence() in KafkaConsumer and KafkaProducer, and mock them in the unit tests. This will allow us to check for the exact value.

Thoughts? @ijuma

Can't you check KafkaConsumer#clientId? We could add a package-private accessor only to be used for testing.

There is no clientId getter currently, but it would indeed be easier if we had a package-private accessor for testing.

Would there be any point in exposing this getter as public?

Good question. I can't think of any right now (maybe someone else can?) so I would lean towards keeping it package-private for now.

cmccabe · 2018-07-31T00:13:04Z

Can you check that the auto-generated client id appears here?

See comment above ^

cmccabe · 2018-07-31T00:13:41Z

@ijuma : I guess you're right; we probably don't need a KIP for this. I left some comments here.

omkreddy · 2018-08-01T06:25:44Z

Can we also update other classes?

KafkaAdminClient.java
KafkaStreams.java
WorkerGroupMember.java

KevinLiLu · 2018-08-01T06:40:23Z

@omkreddy We definitely should. Would you also recommend adding a package-private clientId getter in each of these client classes for testing purposes?

omkreddy · 2018-08-01T07:08:42Z

Since it is a similar change across all classes, I am not sure if we need tests for all classes. KafkaProducer/Consumer test cases may be sufficient. but let others take a call.

omkreddy · 2018-08-01T07:10:42Z

nit: Can't we update and reuse existing MockMetricsReporter class? just trying to avoid adding new class.

Good catch.

KevinLiLu · 2018-08-01T07:21:47Z

Ok, I have added the same logic to KafkaAdminClient, KafkaStreams, and WorkerGroupMember.

The producer & consumer unit tests currently just verify if the auto-generated prefix matches in the unit tests.

omkreddy · 2018-08-01T11:19:43Z

There is a checkstyle error

 Task :clients:checkstyleTest
07:23:27 [ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/kafka-pr-jdk8-scala2.11@2/clients/src/test/java/org/apache/kafka/test/MockMetricsReporter.java:53:79: Empty statement. [EmptyStatement]

KevinLiLu · 2018-08-07T06:19:22Z

@cmccabe @omkreddy

I have added the package-private getters to KafkaProducer and KafkaConsumer so that the tests check the exact client ids now.

What do you guys think?

cmccabe · 2018-08-07T18:32:58Z

This should not be static. We might want to run unit tests in parallel.

Good point. I have pushed a new commit that addresses this.

cmccabe · 2018-08-07T18:33:32Z

generally in assertEquals, the thing being tested comes second. If there is an error message, the first thing appears as the "expected value"

When using assert, the first parameter is expected and second parameter is actual right? In this case, we are testing that the MetricsReporter's client.id will match the correct (expected) client id from the client. The client is the one generating the client.id, and the MetricsReporter needs to receive it correctly.

producer.getClientId() should be first and MockMetricsReporter.clientId should be second?

Sorry, you're right. The thing being tested should be second.

cmccabe · 2018-08-10T16:59:44Z

LGTM. Thanks, @KevinLiLu

cmccabe · 2018-08-10T16:59:55Z

retest this please

KevinLiLu · 2018-08-13T19:17:25Z

retest this please

KevinLiLu · 2018-08-14T00:50:20Z

@hachikuji @rajinisivaram @guozhangwang @ijuma

Can we get a committer to review/merge this?

Thanks @cmccabe for the review!

harshach · 2018-08-23T23:10:05Z

@KevinLiLu overall LGTM. +1 after addressing the minor nits.

KevinLiLu · 2018-08-23T23:44:56Z

@harshach Thanks for the comment! I have addressed the previous comments. Are there any additional changes needed?

harshach · 2018-08-23T23:09:26Z

nit: please add protected keyword

protected gives more visibility than what's there now (package visibility) so not sure why we'd want to do that?

harshach · 2018-08-23T23:09:35Z

nit: please add protected keyword

protected gives more visibility than what's there now (package visibility) so not sure why we'd want to do that?

harshach · 2018-08-30T15:12:05Z

@KevinLiLu sorry I had drafted comments which weren't published earlier. Can you please take a look.

KevinLiLu · 2018-09-11T00:34:08Z

Hi @harshach , is there a particular reason the helper methods for testing should be set to protected?

KevinLiLu · 2018-09-28T02:08:16Z

Hi @lindong28 @hachikuji @rajinisivaram @guozhangwang , can we get a committer to review and close out this minor bug fix? Thanks~

cmccabe · 2018-09-28T17:14:35Z

Any objections to committing this fix and working on any follow-ups in a separate PR?

lindong28 · 2018-09-30T07:25:35Z

@cmccabe If the follow-up is related to whether we should have protected keyword, given that Harsha has not responded and there does not seem to be a reason for protected, I think we can just commit the patch and work on follow-ups in a separate PR if needed.

cmccabe · 2018-10-01T19:34:15Z

retest this please

KevinLiLu · 2018-10-01T21:38:20Z

@cmccabe @lindong28

Seeing an unrelated test failure in kafka.server.DynamicBrokerReconfigurationTest.testUncleanLeaderElectionEnable. And the JDK 8 test seems to be stuck.

Should I do a rebase with trunk? Looks like that test case may have been fixed in a more recent commit.

cmccabe · 2018-10-01T21:57:43Z

Yes, please. Let's rebase on trunk.

KevinLiLu · 2018-10-02T00:34:58Z

@cmccabe

Rebased and now the tests all pass!

cmccabe · 2018-10-03T16:51:56Z

Thanks, @KevinLiLu. LGTM.

@harshach, let's open a follow-on JIRA if there's any more fixups we should make.

Thanks, all.

KevinLiLu · 2018-10-03T16:59:26Z

@cmccabe

Thanks for the merge! Also, congrats on committer!

…che#5383)

KevinLiLu force-pushed the KAFKA-6123 branch from d148230 to d1acab9 Compare July 20, 2018 16:32

cmccabe reviewed Jul 31, 2018

View reviewed changes

omkreddy reviewed Aug 1, 2018

View reviewed changes

KevinLiLu force-pushed the KAFKA-6123 branch from e8f3e95 to 4d974ec Compare August 7, 2018 00:49

cmccabe reviewed Aug 7, 2018

View reviewed changes

KevinLiLu force-pushed the KAFKA-6123 branch from f47751a to c57c70b Compare August 7, 2018 19:11

harshach reviewed Aug 30, 2018

View reviewed changes

KevinLiLu added 8 commits October 1, 2018 15:07

KAFKA-6123: Give client MetricsReporter auto-generated client.id

2b9de3f

Check clientId using startsWith in unit tests

5b9b06a

Use existing MockMetricsReporter for testing

3e354d4

Give auto-generated client.id to all client types

a810d67

Fix checkstyle empty statement

c0dc4ce

Add package-private clientId getters for testing

0c70c05

Make MockMetricsReporter.clientId non-static

01a8844

Remove unused import

ee59e69

KevinLiLu force-pushed the KAFKA-6123 branch from 57ddae0 to ee59e69 Compare October 1, 2018 22:09

cmccabe merged commit a5335e7 into apache:trunk Oct 3, 2018

pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019

KAFKA-6123: Give client MetricsReporter auto-generated client.id (apa…

b1ac25f

…che#5383)

Uh oh!

Conversation

KevinLiLu commented Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

KevinLiLu commented Jul 18, 2018

Uh oh!

cmccabe commented Jul 18, 2018

Uh oh!

KevinLiLu commented Jul 18, 2018

Uh oh!

ijuma commented Jul 20, 2018

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!

cmccabe commented Jul 31, 2018

Uh oh!

omkreddy commented Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KevinLiLu commented Aug 1, 2018

Uh oh!

omkreddy commented Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KevinLiLu commented Aug 1, 2018

Uh oh!

omkreddy commented Aug 1, 2018

Uh oh!

KevinLiLu commented Aug 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KevinLiLu Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KevinLiLu Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmccabe commented Aug 10, 2018

Uh oh!

cmccabe commented Aug 10, 2018

Uh oh!

KevinLiLu commented Aug 13, 2018

Uh oh!

KevinLiLu commented Aug 14, 2018

Uh oh!

harshach commented Aug 23, 2018

Uh oh!

KevinLiLu commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

KevinLiLu commented Jul 18, 2018 •

edited

Loading

omkreddy commented Aug 1, 2018 •

edited

Loading

omkreddy commented Aug 1, 2018 •

edited

Loading

KevinLiLu Aug 7, 2018 •

edited

Loading

KevinLiLu Aug 7, 2018 •

edited

Loading

KevinLiLu commented Aug 23, 2018 •

edited

Loading

KevinLiLu commented Oct 1, 2018 •

edited

Loading