-
Notifications
You must be signed in to change notification settings - Fork 15.4k
KAFKA-12697: Add OfflinePartitionCount and PreferredReplicaImbalanceCount metrics to Quorum Controller #10572
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 15 commits
e411ccc
d1b502b
d65e8e5
4e8aa19
5d672ec
11a1817
b21974c
c35c005
210d3a7
d9df9df
1bee0ff
5a44fcb
070dc3f
f7e1e04
08d213b
53f290c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -285,6 +285,11 @@ public String toString() { | |
| */ | ||
| private final TimelineInteger globalPartitionCount; | ||
|
|
||
| /** | ||
| * A count of the number of partitions that do not have their first replica as a leader. | ||
| */ | ||
| private final TimelineInteger preferredReplicaImbalanceCount; | ||
|
|
||
| /** | ||
| * A reference to the controller's configuration control manager. | ||
| */ | ||
|
|
@@ -330,6 +335,7 @@ public String toString() { | |
| this.controllerMetrics = controllerMetrics; | ||
| this.clusterControl = clusterControl; | ||
| this.globalPartitionCount = new TimelineInteger(snapshotRegistry); | ||
| this.preferredReplicaImbalanceCount = new TimelineInteger(snapshotRegistry); | ||
| this.topicsByName = new TimelineHashMap<>(snapshotRegistry, 0); | ||
| this.topics = new TimelineHashMap<>(snapshotRegistry, 0); | ||
| this.brokersToIsrs = new BrokersToIsrs(snapshotRegistry); | ||
|
|
@@ -366,6 +372,11 @@ public void replay(PartitionRecord record) { | |
| brokersToIsrs.update(record.topicId(), record.partitionId(), prevPartInfo.isr, | ||
| newPartInfo.isr, prevPartInfo.leader, newPartInfo.leader); | ||
| } | ||
| if (newPartInfo.leader != newPartInfo.preferredReplica()) { | ||
| preferredReplicaImbalanceCount.increment(); | ||
| } | ||
| controllerMetrics.setOfflinePartitionCount(brokersToIsrs.offlinePartitionCount()); | ||
| controllerMetrics.setPreferredReplicaImbalanceCount(preferredReplicaImbalanceCount.get()); | ||
| } | ||
|
|
||
| public void replay(PartitionChangeRecord record) { | ||
|
|
@@ -387,6 +398,12 @@ public void replay(PartitionChangeRecord record) { | |
| String topicPart = topicInfo.name + "-" + record.partitionId() + " with topic ID " + | ||
| record.topicId(); | ||
| newPartitionInfo.maybeLogPartitionChange(log, topicPart, prevPartitionInfo); | ||
| if ((newPartitionInfo.leader != newPartitionInfo.preferredReplica()) && | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we have a function like "hasPreferredLeader" on
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can do this in a follow-on |
||
| (prevPartitionInfo.leader == prevPartitionInfo.preferredReplica())) { | ||
| preferredReplicaImbalanceCount.increment(); | ||
| } | ||
| controllerMetrics.setOfflinePartitionCount(brokersToIsrs.offlinePartitionCount()); | ||
| controllerMetrics.setPreferredReplicaImbalanceCount(preferredReplicaImbalanceCount.get()); | ||
| } | ||
|
|
||
| public void replay(RemoveTopicRecord record) { | ||
|
|
@@ -406,11 +423,17 @@ public void replay(RemoveTopicRecord record) { | |
| for (int i = 0; i < partition.isr.length; i++) { | ||
| brokersToIsrs.removeTopicEntryForBroker(topic.id, partition.isr[i]); | ||
| } | ||
| if (partition.leader != partition.preferredReplica()) { | ||
| preferredReplicaImbalanceCount.decrement(); | ||
| } | ||
| globalPartitionCount.decrement(); | ||
| } | ||
| brokersToIsrs.removeTopicEntryForBroker(topic.id, NO_LEADER); | ||
|
|
||
| controllerMetrics.setGlobalTopicsCount(topics.size()); | ||
| controllerMetrics.setGlobalPartitionCount(globalPartitionCount.get()); | ||
| controllerMetrics.setOfflinePartitionCount(brokersToIsrs.offlinePartitionCount()); | ||
| controllerMetrics.setPreferredReplicaImbalanceCount(preferredReplicaImbalanceCount.get()); | ||
| log.info("Removed topic {} with ID {}.", topic.name, record.topicId()); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,11 +21,16 @@ public final class MockControllerMetrics implements ControllerMetrics { | |
| private volatile boolean active; | ||
| private volatile int topics; | ||
| private volatile int partitions; | ||
| private volatile int offlinePartitions; | ||
| private volatile int preferredReplicaImbalances; | ||
|
|
||
|
|
||
| public MockControllerMetrics() { | ||
| this.active = false; | ||
| this.topics = 0; | ||
| this.partitions = 0; | ||
| this.offlinePartitions = 0; | ||
| this.preferredReplicaImbalances = 0; | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -48,7 +53,6 @@ public void updateEventQueueProcessingTime(long durationMs) { | |
| // nothing to do | ||
| } | ||
|
|
||
| @Override | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Weird, did this |
||
| public void setGlobalTopicsCount(int topicCount) { | ||
| this.topics = topicCount; | ||
| } | ||
|
|
@@ -67,4 +71,24 @@ public void setGlobalPartitionCount(int partitionCount) { | |
| public int globalPartitionCount() { | ||
| return this.partitions; | ||
| } | ||
|
|
||
| @Override | ||
| public void setOfflinePartitionCount(int offlinePartitions) { | ||
| this.offlinePartitions = offlinePartitions; | ||
| } | ||
|
|
||
| @Override | ||
| public int offlinePartitionCount() { | ||
| return this.offlinePartitions; | ||
| } | ||
|
|
||
| @Override | ||
| public void setPreferredReplicaImbalanceCount(int replicaImbalances) { | ||
| this.preferredReplicaImbalances = replicaImbalances; | ||
| } | ||
|
|
||
| @Override | ||
| public int preferredReplicaImbalanceCount() { | ||
| return this.preferredReplicaImbalances; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -252,6 +252,73 @@ public void testGlobalTopicAndPartitionMetrics() throws Exception { | |
| assertEquals(0, ctx.metrics.globalPartitionCount()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testOfflinePartitionAndReplicaImbalanceMetrics() throws Exception { | ||
| ReplicationControlTestContext ctx = new ReplicationControlTestContext(); | ||
| ReplicationControlManager replicationControl = ctx.replicationControl; | ||
|
|
||
| for (int i = 0; i < 4; i++) { | ||
| registerBroker(i, ctx); | ||
| unfenceBroker(i, ctx); | ||
| } | ||
|
|
||
| CreatableTopicResult foo = ctx.createTestTopic("foo", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| new int[][] { | ||
| new int[] {0, 2}, | ||
| new int[] {0, 1} | ||
| }); | ||
|
|
||
| CreatableTopicResult zar = ctx.createTestTopic("zar", | ||
| new int[][] { | ||
| new int[] {0, 1, 2}, | ||
| new int[] {1, 2, 3}, | ||
| new int[] {1, 2, 0} | ||
| }); | ||
|
|
||
| ControllerResult<Void> result = replicationControl.unregisterBroker(0); | ||
| ctx.replay(result.records()); | ||
|
|
||
| // All partitions should still be online after unregistering broker 0 | ||
| assertEquals(0, ctx.metrics.offlinePartitionCount()); | ||
| // Three partitions should not have their preferred (first) replica 0 | ||
| assertEquals(3, ctx.metrics.preferredReplicaImbalanceCount()); | ||
|
|
||
| result = replicationControl.unregisterBroker(1); | ||
| ctx.replay(result.records()); | ||
|
|
||
| // After unregistering broker 1, 1 partition for topic foo should go offline | ||
| assertEquals(1, ctx.metrics.offlinePartitionCount()); | ||
| // All five partitions should not have their preferred (first) replica at this point | ||
| assertEquals(5, ctx.metrics.preferredReplicaImbalanceCount()); | ||
|
|
||
| result = replicationControl.unregisterBroker(2); | ||
| ctx.replay(result.records()); | ||
|
|
||
| // After unregistering broker 2, the last partition for topic foo should go offline | ||
| // and 2 partitions for topic zar should go offline | ||
| assertEquals(4, ctx.metrics.offlinePartitionCount()); | ||
|
|
||
| result = replicationControl.unregisterBroker(3); | ||
| ctx.replay(result.records()); | ||
|
|
||
| // After unregistering broker 3 the last partition for topic zar should go offline | ||
| assertEquals(5, ctx.metrics.offlinePartitionCount()); | ||
|
|
||
| // Deleting topic foo should bring the offline partition count down to 3 | ||
| ArrayList<ApiMessageAndVersion> records = new ArrayList<>(); | ||
| replicationControl.deleteTopic(foo.topicId(), records); | ||
| ctx.replay(records); | ||
|
|
||
| assertEquals(3, ctx.metrics.offlinePartitionCount()); | ||
|
|
||
| // Deleting topic zar should bring the offline partition count down to 0 | ||
| records = new ArrayList<>(); | ||
| replicationControl.deleteTopic(zar.topicId(), records); | ||
| ctx.replay(records); | ||
|
|
||
| assertEquals(0, ctx.metrics.offlinePartitionCount()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testValidateNewTopicNames() { | ||
| Map<String, ApiError> topicErrors = new HashMap<>(); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.