KAFKA-12697: Add OfflinePartitionCount and PreferredReplicaImbalanceCount metrics to Quorum Controller #10572

ryandielhenn · 2021-04-21T00:53:42Z

The metrics are calculated by counting records as they are replayed e.g. replay(TopicRecord), replay(RemoveTopicRecord)

This was unit tested using MockControllerMetrics.

https://issues.apache.org/jira/browse/KAFKA-12697

cmccabe · 2021-04-21T02:09:54Z

Hi @dielhennr, thanks for the PR!

I think it would be simpler to have ControllerMetrics#setGlobalTopicsCount / ControllerMetrics#globalTopicsCount rather than inc / dec / get. We can just look at the size of ReplicationControlManager#topics to get the relevant number in O(1) time. This also makes it harder for our count to get out of sync somehow.

For the partition count, we have no choice but to maintain our own count, since we can't efficiently get that number just by taking the size() of any existing data structure. However, we should do this in ReplicationControlManager rather than in the metrics object. There are other reasons why we might want this count besides exposing metrics. For example, one reason is that we might not want to allow more than a certain number of partitions to be created. (There is a KIP proposing such a feature, I believe.) So again for consistency we can just have ControllerMetrics#setGlobalPartitionsCount / ControllerMetrics#globalPartitionsCount

Keep in mind that anything accessed by a metrics callback functions needs to be set in a thread-safe way. The metrics system uses a different set of threads than the controller. So you need to use a lock, a volatile, or an atomic variable when passing information from one to the other. A volatile is usually the best way to do things since it has the lowest overhead (certainly it's the best in this particular case, I think.)

metadata/src/main/java/org/apache/kafka/controller/BrokersToIsrs.java

cmccabe · 2021-05-12T15:36:25Z

Can you create a PR that has just the global topics count and global partitions count? Then we can keep the remaining metrics in this PR. That will make it easier to review and get in.

cmccabe · 2021-05-14T23:25:03Z

metadata/src/main/java/org/apache/kafka/controller/BrokersToIsrs.java

     */
    private final TimelineHashMap<Integer, TimelineHashMap<Uuid, int[]>> isrMembers;
+
+    private final Map<Uuid, Integer> offlinePartitionCounts;


We cannot use regular maps here because they will not roll back to the desired state during a snapshot restore.

In any case, I don't see why we need this map. It's enough to know how many offline partitions there are, which we already have a count of below.

This was so that when a topic is removed, any offline partitions for that topic are decremented from the counter.

All the information that is needed is already here. If you delete X partitions that had a leader of -1, you decrement the counter by X.

cmccabe · 2021-05-20T22:54:04Z

metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java

        String topicPart = topicInfo.name + "-" + record.partitionId() + " with topic ID " +
            record.topicId();
        newPartitionInfo.maybeLogPartitionChange(log, topicPart, prevPartitionInfo);
+        if ((newPartitionInfo.leader != newPartitionInfo.preferredReplica()) && 


Can we have a function like "hasPreferredLeader" on PartitionControlInfo, to make this simpler?

Can do this in a follow-on

cmccabe · 2021-05-20T23:06:04Z

metadata/src/test/java/org/apache/kafka/controller/ReplicationControlManagerTest.java

+            unfenceBroker(i, ctx);
+        }
+
+        CreatableTopicResult foo = ctx.createTestTopic("foo", 


This is definitely a nitpick, but can you put this one fewer lines? 2 lines should be enough for this. Same for the ones below.

cmccabe · 2021-05-20T23:07:36Z

metadata/src/test/java/org/apache/kafka/controller/MockControllerMetrics.java

        // nothing to do
    }

-    @Override


Weird, did this @Override line get deleted by accident?

cmccabe

LGTM

…e-allocations-lz4 * apache-github/trunk: (43 commits) KAFKA-12800: Configure generator to fail on trailing JSON tokens (apache#10717) MINOR: clarify message ordering with max in-flight requests and idempotent producer (apache#10690) MINOR: Add log identifier/prefix printing in Log layer static functions (apache#10742) MINOR: update java doc for deprecated methods (apache#10722) MINOR: Fix deprecation warnings in SlidingWindowedCogroupedKStreamImplTest (apache#10703) KAFKA-12499: add transaction timeout verification (apache#10482) KAFKA-12620 Allocate producer ids on the controller (apache#10504) MINOR: Kafka Streams code samples formating unification (apache#10651) KAFKA-12808: Remove Deprecated Methods under StreamsMetrics (apache#10724) KAFKA-12522: Cast SMT should allow null value records to pass through (apache#10375) KAFKA-12820: Upgrade maven-artifact dependency to resolve CVE-2021-26291 HOTFIX: fix checkstyle issue in KAFKA-12697 KAFKA-12697: Add OfflinePartitionCount and PreferredReplicaImbalanceCount metrics to Quorum Controller (apache#10572) KAFKA-12342: Remove MetaLogShim and use RaftClient directly (apache#10705) KAFKA-12779: KIP-740, Clean up public API in TaskId and fix TaskMetadata#taskId() (apache#10735) KAFKA-12814: Remove Deprecated Method StreamsConfig getConsumerConfigs (apache#10737) MINOR: Eliminate redundant functions in LogTest suite (apache#10732) MINOR: Remove unused maxProducerIdExpirationMs parameter in Log constructor (apache#10723) MINOR: Updating files with release 2.7.1 (apache#10660) KAFKA-12809: Remove deprecated methods of Stores factory (apache#10729) ...

ryandielhenn added 2 commits April 20, 2021 16:41

global-topic-count-metric-and-test

e411ccc

add-global-partition-and-topic-metrics

d1b502b

ryandielhenn added 3 commits April 21, 2021 10:15

comments-addressed

d65e8e5

merge trunk

4e8aa19

Merge remote-tracking branch 'origin/trunk' into KAFKA-12697

5d672ec

ryandielhenn changed the title ~~Add GlobalTopicCount and GlobalPartitionCount metrics to Quorum Controller~~ KAFKA-12697 Add GlobalTopicCount and GlobalPartitionCount metrics to Quorum Controller May 7, 2021

ryandielhenn changed the title ~~KAFKA-12697 Add GlobalTopicCount and GlobalPartitionCount metrics to Quorum Controller~~ KAFKA-12697: Add GlobalTopicCount and GlobalPartitionCount metrics to Quorum Controller May 7, 2021

ryandielhenn changed the title ~~KAFKA-12697: Add GlobalTopicCount and GlobalPartitionCount metrics to Quorum Controller~~ KAFKA-12697: Add metrics to Quorum Controller May 7, 2021

ryandielhenn added 2 commits May 10, 2021 10:45

Merge remote-tracking branch 'origin/trunk' into KAFKA-12697

11a1817

adding-offline-partition-and-replica-imbalance-metrics

b21974c

cmccabe reviewed May 12, 2021

View reviewed changes

metadata/src/main/java/org/apache/kafka/controller/BrokersToIsrs.java Show resolved Hide resolved

address-comments

c35c005

ryandielhenn force-pushed the KAFKA-12697 branch from 19b6e20 to c35c005 Compare May 12, 2021 21:41

ryandielhenn added 4 commits May 13, 2021 12:19

moving-q-size

210d3a7

merge-trunk

d9df9df

nit

1bee0ff

nit

5a44fcb

ryandielhenn changed the title ~~KAFKA-12697: Add metrics to Quorum Controller~~ KAFKA-12697: Add OfflinePartitionCount and PreferredReplicaImbalanceCount metrics to Quorum Controller May 13, 2021

cmccabe reviewed May 14, 2021

View reviewed changes

ryandielhenn added 3 commits May 17, 2021 11:48

address-comments

070dc3f

merge-trunk

f7e1e04

using-timeline-int

08d213b

cmccabe reviewed May 20, 2021

View reviewed changes

address-comments

53f290c

cmccabe approved these changes May 20, 2021

View reviewed changes

cmccabe merged commit 80ec8fb into apache:trunk May 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

KAFKA-12697: Add OfflinePartitionCount and PreferredReplicaImbalanceCount metrics to Quorum Controller #10572

KAFKA-12697: Add OfflinePartitionCount and PreferredReplicaImbalanceCount metrics to Quorum Controller #10572

Uh oh!

ryandielhenn commented Apr 21, 2021 •

edited

Loading

Uh oh!

cmccabe commented Apr 21, 2021 •

edited

Loading

Uh oh!

Uh oh!

cmccabe commented May 12, 2021

Uh oh!

cmccabe May 14, 2021

Uh oh!

ryandielhenn May 14, 2021

Uh oh!

cmccabe May 17, 2021 •

edited

Loading

Uh oh!

cmccabe May 20, 2021

Uh oh!

cmccabe May 20, 2021

Uh oh!

cmccabe May 20, 2021

Uh oh!

cmccabe May 20, 2021 •

edited

Loading

Uh oh!

cmccabe left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

KAFKA-12697: Add OfflinePartitionCount and PreferredReplicaImbalanceCount metrics to Quorum Controller #10572

KAFKA-12697: Add OfflinePartitionCount and PreferredReplicaImbalanceCount metrics to Quorum Controller #10572

Uh oh!

Conversation

ryandielhenn commented Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmccabe commented Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cmccabe commented May 12, 2021

Uh oh!

cmccabe May 14, 2021

Choose a reason for hiding this comment

Uh oh!

ryandielhenn May 14, 2021

Choose a reason for hiding this comment

Uh oh!

cmccabe May 17, 2021 • 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!

cmccabe May 20, 2021

Choose a reason for hiding this comment

Uh oh!

cmccabe May 20, 2021

Choose a reason for hiding this comment

Uh oh!

cmccabe May 20, 2021

Choose a reason for hiding this comment

Uh oh!

cmccabe May 20, 2021 • 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!

cmccabe left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ryandielhenn commented Apr 21, 2021 •

edited

Loading

cmccabe commented Apr 21, 2021 •

edited

Loading

cmccabe May 17, 2021 •

edited

Loading

cmccabe May 20, 2021 •

edited

Loading