Skip to content

KAFKA-6263: Expose metrics for group and transaction metadata loading duration#7045

Closed
anatasiavela wants to merge 9 commits into
apache:trunkfrom
anatasiavela:KAFKA-6263
Closed

KAFKA-6263: Expose metrics for group and transaction metadata loading duration#7045
anatasiavela wants to merge 9 commits into
apache:trunkfrom
anatasiavela:KAFKA-6263

Conversation

@anatasiavela

Copy link
Copy Markdown
Contributor

JIRA

  • Add metrics to provide visibility for how long group metadata and transaction metadata take to load in order to understand some inactivity seen in the consumer groups
  • Tests include mocking load times by creating a delay after each are loaded and ensuring the measured JMX metric is as it should be

Committer Checklist (excluded from commit message)

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

@wushujames

Copy link
Copy Markdown
Contributor

Can you also update the metrics documentation with these new metrics? They are at https://github.com/apache/kafka/blob/trunk/docs/ops.html

@wushujames wushujames left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks!

Comment thread core/src/test/scala/unit/kafka/coordinator/group/GroupMetadataManagerTest.scala Outdated
Comment thread core/src/main/scala/kafka/coordinator/group/GroupMetadataManager.scala Outdated

@gwenshap gwenshap left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The metrics look good, but the test should be simpler and shorter. I gave some suggestions.

Comment thread core/src/main/scala/kafka/coordinator/group/GroupMetadataManager.scala Outdated
Comment thread core/src/test/scala/unit/kafka/coordinator/group/GroupMetadataManagerTest.scala Outdated
Comment thread core/src/test/scala/unit/kafka/coordinator/group/GroupMetadataManagerTest.scala Outdated
Comment thread core/src/test/scala/unit/kafka/coordinator/group/GroupMetadataManagerTest.scala Outdated

@hachikuji hachikuji left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@anatasiavela Thanks for the contribution! I just had a few nitpicks.

Comment thread core/src/main/scala/kafka/coordinator/group/GroupMetadataManager.scala Outdated
Comment thread core/src/test/scala/unit/kafka/coordinator/group/GroupMetadataManagerTest.scala Outdated
Comment thread core/src/test/scala/unit/kafka/coordinator/group/GroupMetadataManagerTest.scala Outdated
Comment thread core/src/test/scala/unit/kafka/coordinator/group/GroupMetadataManagerTest.scala Outdated
val reporter = new JmxReporter("kafka.server")
metrics.addReporter(reporter)

def partitionLoadTime(attribute: String): Double = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice cleanup

@gwenshap gwenshap left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. I'll wait for tests to pass before merging.

@hachikuji hachikuji left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the contribution! LGTM

@gwenshap

gwenshap commented Aug 2, 2019

Copy link
Copy Markdown
Contributor

retest this please

@gwenshap

gwenshap commented Aug 4, 2019

Copy link
Copy Markdown
Contributor

ok, passed enough. Merging. Thank you for contributing, Anastasia!

@gwenshap gwenshap closed this in 0b1dc1c Aug 4, 2019
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.

5 participants