KAFKA-9152; Improve Sensor Retrieval by highluck · Pull Request #7928 · apache/kafka

highluck · 2020-01-10T16:13:44Z

This ticket shall improve two aspects of the retrieval of sensors:
https://issues.apache.org/jira/browse/KAFKA-9152

Currently, when a sensor is retrieved with *Metrics.*Sensor() (e.g. ThreadMetrics.createTaskSensor()) after it was created with the same method *Metrics.*Sensor(), the sensor is added again to the corresponding queue in Sensors (e.g. threadLevelSensors) in StreamsMetricsImpl. Those queues are used to remove the sensors when removeAllLevelSensors() is called. Having multiple times the same sensors in this queue is not an issue from a correctness point of view. However, it would reduce the footprint to only store a sensor once in those queues.
When a sensor is retrieved, the current code attempts to create a new sensor and to add to it again the corresponding metrics. This could be avoided.

Both aspects could be improved by checking whether a sensor already exists by calling getSensor() on the Metrics object and checking the return value.

Committer Checklist (excluded from commit message)

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

cadonna

@highluck Thanks again for the PR

Out of curiosity, why did you close PR #7914? You know that you just need to push to the same branch to update a PR, don't you? No need to close one PR and open a new one. If you open a new PR for each version we easily lose track of the comments.

highluck · 2020-01-21T14:42:03Z

@cadonna
Thank you for review
closing the PR was my mistake

I learned a lot from your reviews

Thank you very much :)

i'm code update!!

cadonna

The PR is looking better. I have still some comments.

highluck · 2020-01-22T00:15:38Z

@cadonna Thank you!!
i'm code update!

cadonna

The PR looks good to me. I have just a few minor comments.

cadonna · 2020-01-22T02:14:32Z

Call for final review: @bbejeck @mjsax @guozhangwang @vvcephei

Does anybody of you know why the builds are not triggered for this PR?

cadonna · 2020-01-22T02:43:09Z

@highluck Could you please better describe this PR in the PR message at the top of this page underneath the PR title?

guozhangwang · 2020-01-22T03:47:02Z

retest this please

highluck · 2020-01-22T03:48:40Z

@cadonna @guozhangwang
thank you code update

bbejeck · 2020-01-22T20:21:37Z

Does anybody of you know why the builds are not triggered for this PR?

@cadonna I'm looking into it. It seems the builds may get triggered, but for some reason, the Github page isn't updating.

Retest this please.

cadonna · 2020-01-22T21:18:58Z

Retest this please

bbejeck

Thanks for the PR @highluck, overall LGTM I just have minor questions about the tests and one parameter needs to get marked as final.

bbejeck · 2020-01-22T20:59:25Z

Should be final here.

bbejeck · 2020-01-22T21:25:09Z

Why is(equalToObject(sensor))) and not just is(sensor)?

That is on me. In this verification it is important to check for reference equality, because you want that threadLevelSensor() returns the sensor that is registered in the Metrics object or that was created by Metrics#sensor().

Yeah, I get that we want to make sure the same instance is returned. But since Sensor doesn't override equals, is(sensor) should still do an instance equality check. It's really a minor point, so I don't care too much if we keep it as is.

I used equalToObject() because it makes the intent more explicit.

Fair enough, let's just leave it as is then. Thanks for the explanation.

bbejeck · 2020-01-22T21:25:18Z

bbejeck · 2020-01-22T21:25:28Z

ditto and same for tests below, I won't repeat this comment.

bbejeck · 2020-01-22T22:05:55Z

retest this please

bbejeck · 2020-01-22T22:20:06Z

@highluck the failure is related Execution failed for task ':streams:checkstyleTest'.

See comment #7928 (comment) above

highluck · 2020-01-22T23:43:46Z

@bbejeck
thank you
i'm code update!!

bbejeck · 2020-01-23T01:20:51Z

Retest this please.

highluck · 2020-01-23T03:51:07Z

@bbejeck
I don't know why there's an build error.
java.lang.AssertionError: Condition not met within timeout 15000. Failed to observe stream transits to RUNNING

cadonna · 2020-01-23T18:49:56Z

@highluck Hard to say, but it seems like a flaky test. What test does throw this exception?

highluck · 2020-01-23T21:43:29Z

@cadonna
It's just a jenkins log! about this PR
org.apache.kafka.streams.integration.BranchedMultiLevelRepartitionConnectedTopologyTest.testTopologyBuild

But there is no problem in my local test

highluck · 2020-01-24T01:14:15Z

Retest this please

bbejeck · 2020-01-24T01:38:03Z

Retest this please

bbejeck · 2020-01-24T14:45:00Z

Java 8 failed with org.apache.kafka.connect.mirror.MirrorConnectorsIntegrationTest.testReplication

Java 11 passed

Retest this please.

highluck · 2020-01-24T18:59:32Z

@bbejeck
Retest this please.

cadonna · 2020-01-24T19:05:54Z

JDK 11/Scala 2.13 failed with

kafka.admin.ReassignPartitionsClusterTest.shouldListMovingPartitionsThroughApi
kafka.api.ConsumerBounceTest.testRollingBrokerRestartsWithSmallerMaxGroupSizeConfigDisruptsBigGroup
kafka.network.SocketServerTest.testIdleConnection

JDK 8/Scala 2.12 failed with Jenkins specific error

fatal: unable to create thread: Resource temporarily unavailable

bbejeck · 2020-01-24T19:34:23Z

Retest this please.

bbejeck · 2020-01-24T21:56:11Z

Merged #7928 into trunk.

bbejeck · 2020-01-24T21:56:26Z

Thanks @highluck for the contribution!

Conflicts and/or compiler errors due to the fact that we temporarily reverted the commit that removes Scala 2.11 support: * Exit.scala: replace SAMs with anonymous inner classes. * MiniKdc.scala: take upstream changes. # By A. Sophie Blee-Goldman (1) and others # Via Jason Gustafson * apache-github/trunk: KAFKA-9254; Overridden topic configs are reset after dynamic default change (apache#7870) MINOR: MiniKdc JVM shutdown hook fix (apache#7946) KAFKA-9152; Improve Sensor Retrieval (apache#7928) Correct exception message in DistributedHerder (apache#7995) KAFKA-7317: Use collections subscription for main consumer to reduce metadata (apache#7969) KAFKA-9181; Maintain clean separation between local and group subscriptions in consumer's SubscriptionState (apache#7941) KAFKA-7737; Use single path in producer for initializing the producerId (apache#7920) # Conflicts: # core/src/test/scala/kafka/security/minikdc/MiniKdc.scala

cadonna reviewed Jan 21, 2020

View reviewed changes

Comment thread ...s/src/main/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImpl.java Outdated

Comment thread ...c/test/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImplTest.java Outdated

highluck force-pushed the improve-sensor-re branch from 1929d2d to b8fb712 Compare January 21, 2020 14:37

cadonna reviewed Jan 21, 2020

View reviewed changes

highluck force-pushed the improve-sensor-re branch from b8fb712 to acfd29d Compare January 22, 2020 00:15

cadonna approved these changes Jan 22, 2020

View reviewed changes

Comment thread ...c/test/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImplTest.java Outdated

Comment thread ...c/test/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImplTest.java Outdated

highluck force-pushed the improve-sensor-re branch from acfd29d to 0dde9df Compare January 22, 2020 03:48

highluck force-pushed the improve-sensor-re branch from 0dde9df to a6a5568 Compare January 22, 2020 03:50

bbejeck reviewed Jan 22, 2020

View reviewed changes

KAFKA-9152; Improve Sensor Retrieval

e55a741

highluck force-pushed the improve-sensor-re branch from f979076 to e55a741 Compare January 22, 2020 23:42

highluck requested a review from bbejeck January 23, 2020 14:36

bbejeck approved these changes Jan 24, 2020

View reviewed changes

bbejeck merged commit 2e351e0 into apache:trunk Jan 24, 2020

Conversation

highluck commented Jan 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

highluck commented Jan 21, 2020

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

highluck commented Jan 22, 2020

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cadonna commented Jan 22, 2020

Uh oh!

cadonna commented Jan 22, 2020

Uh oh!

guozhangwang commented Jan 22, 2020

Uh oh!

highluck commented Jan 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bbejeck commented Jan 22, 2020

Uh oh!

cadonna commented Jan 22, 2020

Uh oh!

bbejeck left a comment • 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!

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!

bbejeck commented Jan 22, 2020

Uh oh!

bbejeck commented Jan 22, 2020

Uh oh!

highluck commented Jan 22, 2020

Uh oh!

bbejeck commented Jan 23, 2020

Uh oh!

highluck commented Jan 23, 2020

Uh oh!

cadonna commented Jan 23, 2020

Uh oh!

highluck commented Jan 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highluck commented Jan 24, 2020

Uh oh!

bbejeck commented Jan 24, 2020

Uh oh!

bbejeck commented Jan 24, 2020

highluck commented Jan 10, 2020 •

edited

Loading

highluck commented Jan 22, 2020 •

edited

Loading

bbejeck left a comment •

edited

Loading

highluck commented Jan 23, 2020 •

edited

Loading

highluck commented Jan 24, 2020 •

edited

Loading