Skip to content

MINOR: Remove deprecated per-partition lag metrics#5172

Closed
lindong28 wants to merge 4 commits into
apache:trunkfrom
lindong28:remove-deprecated-metrics
Closed

MINOR: Remove deprecated per-partition lag metrics#5172
lindong28 wants to merge 4 commits into
apache:trunkfrom
lindong28:remove-deprecated-metrics

Conversation

@lindong28

@lindong28 lindong28 commented Jun 9, 2018

Copy link
Copy Markdown
Member

It takes O(n^2) time to instantiate a mbean with n attributes which can be very slow if the number of attributes of this mbean is large. This PR removes metrics whose number of attributes can grow with the number of partitions in the cluster to fix the performance issue. These metrics have already been marked for removal in 2.0 by KIP-225.

Committer Checklist (excluded from commit message)

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

@lindong28

Copy link
Copy Markdown
Member Author

The code comment says that we intend to remove those metrics in Kafka 2.0. Since we have already release 2.0, it is probably better to remove deprecated metrics sooner than later.

@becketqin @hachikuji Do you have time to review this patch?

@lindong28

Copy link
Copy Markdown
Member Author

@rajinisivaram Since it is better to remove deprecated metrics sooner than later, do you think we should include this patch in Kafka 2.0?

@lindong28

Copy link
Copy Markdown
Member Author

The motivation to remove those metrics is that they can significantly delay the startup time of large consumers (e.g. MM) for minutes as explained in #5011.

@lindong28
lindong28 force-pushed the remove-deprecated-metrics branch from 3e3b790 to 1721b3a Compare June 9, 2018 01:01
@ijuma

ijuma commented Jun 9, 2018

Copy link
Copy Markdown
Member

Yes, let's do this. But we need to add an upgrade note.

@ijuma ijuma added this to the 2.0.0 milestone Jun 9, 2018
@ijuma

ijuma commented Jun 9, 2018

Copy link
Copy Markdown
Member

There are a couple of failing tests.

@lindong28
lindong28 force-pushed the remove-deprecated-metrics branch from be9ce24 to 9e6bb40 Compare June 9, 2018 23:40
@lindong28

Copy link
Copy Markdown
Member Author

@ijuma Thanks for the review. The upgrade note is updated and all tests have passed.

@ijuma ijuma left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

I would normally suggest not removing things so quickly (the deprecation happened in 1.1), but since these metrics can cause a major performance issue and the KIP specifically said that they would be removed in 2.0.0, let's do it.

@lindong28, can you please mention in the commit message that these metrics were marked for removal in 2.0 by KIP-225 and that they can be a performance issue if the number of partitions is large.

Comment thread docs/upgrade.html Outdated
JMX monitoring tools that do not automatically aggregate. To get the total count for a specific request type, the tool needs to be
updated to aggregate across different versions.
</li>
<li><a href="https://cwiki.apache.org/confluence/x/uaBzB">KIP-225</a> changed the metric "records.lag" to use tags for topic and partition. The original version with the name format "{topic}-{partition}.records-lag" is removed.</li>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: is removed should be has been removed.

lindong28 added a commit that referenced this pull request Jun 12, 2018
It takes O(n^2) time to instantiate a mbean with n attributes which can be very slow if the number of attributes of this mbean is large. This PR removes metrics whose number of attributes can grow with the number of partitions in the cluster to fix the performance issue. These metrics have already been marked for removal in 2.0 by KIP-225.

Author: Dong Lin <lindong28@gmail.com>

Reviewers: Ismael Juma <ismael@juma.me.uk>

Closes #5172 from lindong28/remove-deprecated-metrics

(cherry picked from commit 4580d9f)
Signed-off-by: Dong Lin <lindong28@gmail.com>
@lindong28

Copy link
Copy Markdown
Member Author

@ijuma Thanks for the review. The patch and the description has been updated as suggested. PR has been merged to trunk.

@lindong28 lindong28 closed this in 4580d9f Jun 12, 2018
@ijuma

ijuma commented Jun 12, 2018

Copy link
Copy Markdown
Member

Please cherry-pick to 2.0 as well.

@lindong28

Copy link
Copy Markdown
Member Author

@ijuma Certainly. It has been cherry-picked into 2.0 branch.

@lindong28
lindong28 deleted the remove-deprecated-metrics branch June 21, 2018 19:14
ying-zheng pushed a commit to ying-zheng/kafka that referenced this pull request Jul 6, 2018
It takes O(n^2) time to instantiate a mbean with n attributes which can be very slow if the number of attributes of this mbean is large. This PR removes metrics whose number of attributes can grow with the number of partitions in the cluster to fix the performance issue. These metrics have already been marked for removal in 2.0 by KIP-225.

Author: Dong Lin <lindong28@gmail.com>

Reviewers: Ismael Juma <ismael@juma.me.uk>

Closes apache#5172 from lindong28/remove-deprecated-metrics
akatona84 pushed a commit to akatona84/kafka that referenced this pull request Jul 30, 2018
…r than using metric name.

This is the implementation of KIP-225.
It marks the previous metrics as deprecated in the documentation and adds new metrics using tags.

Testing verifies that both the new and the old metric report the same value.

Author: cmolter <cmolter@apple.com>

Reviewers: Jiangjie (Becket) Qin <becket.qin@gmail.com>

Closes apache#4362 from lahabana/kafka-5890

KAFKA-6184; report a metric of the lag between the consumer offset ...

Add `records-lead` and partition-level `{topic}-{partition}.records-lead-min|avg` for fetcher metrics.

junrao  Please kindly review. Thanks.

Author: huxihx <huxi_2b@hotmail.com>

Reviewers: Jun Rao <junrao@gmail.com>

Closes apache#4191 from huxihx/KAFKA-6184

MINOR: Remove deprecated per-partition lag metrics

It takes O(n^2) time to instantiate a mbean with n attributes which can be very slow if the number of attributes of this mbean is large. This PR removes metrics whose number of attributes can grow with the number of partitions in the cluster to fix the performance issue. These metrics have already been marked for removal in 2.0 by KIP-225.

Author: Dong Lin <lindong28@gmail.com>

Reviewers: Ismael Juma <ismael@juma.me.uk>

Closes apache#5172 from lindong28/remove-deprecated-metrics

(cherry picked from commit 4580d9f)
Signed-off-by: Dong Lin <lindong28@gmail.com>

CLOUDERA-BUILD: Fix compilation error

Use TestUtils.createTopic in PlaintextConsumerTest

Change-Id: Ia3296e3093039c11b5c5c264a476ce51211a01f7
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.

2 participants