-
Notifications
You must be signed in to change notification settings - Fork 15.4k
KAFKA-15586: Clean shutdown detection - server side #14706
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 13 commits
cf035f5
38aa224
2a00681
9994d45
525a74c
f5ac59d
9242fa9
4484ffd
b955bda
178712f
01352be
ffa75a9
fd8842e
350ecd4
02958b1
3eb7696
58eeb7d
fe8e116
5f850a3
e406e0f
c0eedce
d030708
18dd798
cb7e41a
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 |
|---|---|---|
| @@ -0,0 +1,162 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.kafka.controller; | ||
|
|
||
| import org.apache.kafka.common.Uuid; | ||
| import org.apache.kafka.metadata.Replicas; | ||
| import org.apache.kafka.timeline.SnapshotRegistry; | ||
| import org.apache.kafka.timeline.TimelineHashMap; | ||
|
|
||
| import java.util.Arrays; | ||
| import java.util.Collections; | ||
| import java.util.Map; | ||
|
|
||
| import static org.apache.kafka.metadata.Replicas.NONE; | ||
|
|
||
| public class BrokersToElrs { | ||
| private final SnapshotRegistry snapshotRegistry; | ||
|
|
||
| // It maps from the broker id to the topic id partitions if the partition has ELR. | ||
| private final TimelineHashMap<Integer, TimelineHashMap<Uuid, int[]>> elrMembers; | ||
|
CalvinLiu7947 marked this conversation as resolved.
|
||
|
|
||
| BrokersToElrs(SnapshotRegistry snapshotRegistry) { | ||
| this.snapshotRegistry = snapshotRegistry; | ||
| this.elrMembers = new TimelineHashMap<>(snapshotRegistry, 0); | ||
| } | ||
|
|
||
| /** | ||
| * Update our records of a partition's ELR. | ||
| * | ||
| * @param topicId The topic ID of the partition. | ||
| * @param partitionId The partition ID of the partition. | ||
| * @param prevElr The previous ELR, or null if the partition is new. | ||
| * @param nextElr The new ELR, or null if the partition is being removed. | ||
| */ | ||
|
|
||
| void update(Uuid topicId, int partitionId, int[] prevElr, int[] nextElr) { | ||
| int[] prev; | ||
| if (prevElr == null) { | ||
| prev = NONE; | ||
| } else { | ||
| prev = Replicas.clone(prevElr); | ||
| Arrays.sort(prev); | ||
| } | ||
| int[] next; | ||
| if (nextElr == null) { | ||
| next = NONE; | ||
| } else { | ||
| next = Replicas.clone(nextElr); | ||
| Arrays.sort(next); | ||
| } | ||
|
|
||
| int i = 0, j = 0; | ||
| while (true) { | ||
| if (i == prev.length) { | ||
| if (j == next.length) { | ||
| break; | ||
| } | ||
| int newReplica = next[j]; | ||
| add(newReplica, topicId, partitionId); | ||
| j++; | ||
| } else if (j == next.length) { | ||
| int prevReplica = prev[i]; | ||
| remove(prevReplica, topicId, partitionId); | ||
| i++; | ||
| } else { | ||
| int prevReplica = prev[i]; | ||
| int newReplica = next[j]; | ||
| if (prevReplica < newReplica) { | ||
| remove(prevReplica, topicId, partitionId); | ||
| i++; | ||
| } else if (prevReplica > newReplica) { | ||
| add(newReplica, topicId, partitionId); | ||
| j++; | ||
| } else { | ||
| i++; | ||
| j++; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| void removeTopicEntryForBroker(Uuid topicId, int brokerId) { | ||
| Map<Uuid, int[]> topicMap = elrMembers.get(brokerId); | ||
| if (topicMap != null) { | ||
| topicMap.remove(topicId); | ||
| } | ||
| } | ||
|
|
||
| private void add(int brokerId, Uuid topicId, int newPartition) { | ||
| TimelineHashMap<Uuid, int[]> topicMap = elrMembers.get(brokerId); | ||
| if (topicMap == null) { | ||
| topicMap = new TimelineHashMap<>(snapshotRegistry, 0); | ||
| elrMembers.put(brokerId, topicMap); | ||
| } | ||
| int[] partitions = topicMap.get(topicId); | ||
| int[] newPartitions; | ||
| if (partitions == null) { | ||
| newPartitions = new int[1]; | ||
| } else { | ||
| newPartitions = new int[partitions.length + 1]; | ||
| System.arraycopy(partitions, 0, newPartitions, 0, partitions.length); | ||
| } | ||
| newPartitions[newPartitions.length - 1] = newPartition; | ||
| topicMap.put(topicId, newPartitions); | ||
| } | ||
|
|
||
| private void remove(int brokerId, Uuid topicId, int removedPartition) { | ||
| TimelineHashMap<Uuid, int[]> topicMap = elrMembers.get(brokerId); | ||
| if (topicMap == null) { | ||
| throw new RuntimeException("Broker " + brokerId + " has no elrMembers " + | ||
| "entry, so we can't remove " + topicId + ":" + removedPartition); | ||
| } | ||
| int[] partitions = topicMap.get(topicId); | ||
| if (partitions == null) { | ||
| throw new RuntimeException("Broker " + brokerId + " has no " + | ||
| "entry in elrMembers for topic " + topicId); | ||
| } | ||
| if (partitions.length == 1) { | ||
| if (partitions[0] != removedPartition) { | ||
| throw new RuntimeException("Broker " + brokerId + " has no " + | ||
| "entry in elrMembers for " + topicId + ":" + removedPartition); | ||
| } | ||
| topicMap.remove(topicId); | ||
| if (topicMap.isEmpty()) { | ||
| elrMembers.remove(brokerId); | ||
| } | ||
| } else { | ||
| int[] newPartitions = new int[partitions.length - 1]; | ||
| int j = 0; | ||
| for (int i = 0; i < partitions.length; i++) { | ||
| int partition = partitions[i]; | ||
| if (partition != removedPartition) { | ||
| newPartitions[j++] = partition; | ||
| } | ||
| } | ||
| topicMap.put(topicId, newPartitions); | ||
| } | ||
| } | ||
|
|
||
| BrokersToIsrs.PartitionsOnReplicaIterator partitionsWithBrokerInElr(int brokerId) { | ||
| Map<Uuid, int[]> topicMap = elrMembers.get(brokerId); | ||
| if (topicMap == null) { | ||
| topicMap = Collections.emptyMap(); | ||
| } | ||
| return new BrokersToIsrs.PartitionsOnReplicaIterator(topicMap, false); | ||
| } | ||
| } | ||
|
CalvinLiu7947 marked this conversation as resolved.
Outdated
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -247,6 +247,8 @@ boolean check() { | |
| */ | ||
| private final boolean zkMigrationEnabled; | ||
|
|
||
| private HandleBrokerUncleanShutdownHelper handleBrokerUncleanShutdownHelper; | ||
|
|
||
| /** | ||
| * Maps controller IDs to controller registrations. | ||
| */ | ||
|
|
@@ -304,6 +306,10 @@ public void deactivate() { | |
| heartbeatManager = null; | ||
| } | ||
|
|
||
| public void setHandleBrokerUncleanShutdownHelper(HandleBrokerUncleanShutdownHelper handleBrokerUncleanShutdownHelper) { | ||
| this.handleBrokerUncleanShutdownHelper = handleBrokerUncleanShutdownHelper; | ||
| } | ||
|
|
||
|
CalvinLiu7947 marked this conversation as resolved.
Outdated
|
||
| Map<Integer, BrokerRegistration> brokerRegistrations() { | ||
| return brokerRegistrations; | ||
| } | ||
|
|
@@ -336,10 +342,14 @@ public ControllerResult<BrokerRegistrationReply> registerBroker( | |
| ", but got cluster ID " + request.clusterId()); | ||
| } | ||
| int brokerId = request.brokerId(); | ||
| List<ApiMessageAndVersion> records = new ArrayList<>(); | ||
| BrokerRegistration existing = brokerRegistrations.get(brokerId); | ||
| if (version < 2 || existing == null || request.previousBrokerEpoch() != existing.epoch()) { | ||
| // TODO(KIP-966): Update the ELR if the broker has an unclean shutdown. | ||
| log.debug("Received an unclean shutdown request"); | ||
|
CalvinLiu7947 marked this conversation as resolved.
CalvinLiu7947 marked this conversation as resolved.
|
||
| if (handleBrokerUncleanShutdownHelper == null) { | ||
| log.warn("No handleBrokerUncleanShutdownHelper provided"); | ||
|
Member
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. Related to the comment about the builder, we should do this validation when constructing the class so we can throw exceptions on startup if the handler was somehow not specified. To work around the circular dependency of ClusterControlManager and ReplicaManager, you can add something like this to QuorumController: and use that method in the builder of ClusterControlManager.
Contributor
Author
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. Thanks for the advice! Updated. |
||
| } else { | ||
| handleBrokerUncleanShutdownHelper.apply(request.brokerId(), records); | ||
| } | ||
| } | ||
| if (existing != null) { | ||
| if (heartbeatManager.hasValidSession(brokerId)) { | ||
|
|
@@ -410,7 +420,6 @@ public ControllerResult<BrokerRegistrationReply> registerBroker( | |
|
|
||
| heartbeatManager.register(brokerId, record.fenced()); | ||
|
|
||
| List<ApiMessageAndVersion> records = new ArrayList<>(); | ||
| records.add(new ApiMessageAndVersion(record, featureControl.metadataVersion(). | ||
| registerBrokerRecordVersion())); | ||
| return ControllerResult.atomicOf(records, new BrokerRegistrationReply(brokerEpoch)); | ||
|
|
@@ -780,4 +789,9 @@ public Entry<Integer, Map<String, VersionRange>> next() { | |
| } | ||
| }; | ||
| } | ||
|
|
||
| @FunctionalInterface | ||
| interface HandleBrokerUncleanShutdownHelper { | ||
|
CalvinLiu7947 marked this conversation as resolved.
Outdated
|
||
| void apply(int brokerId, List<ApiMessageAndVersion> records); | ||
|
CalvinLiu7947 marked this conversation as resolved.
Outdated
|
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To help review this file, you can refer to the BrokersToIsrs.java
The reasons not to modify the BrokersToIsrs are:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the alternative would be to extend the BrokersToIsrs to include a ELR_FLAG similar to the LEADER_FLAG. I agree this would probably be complex to implement. If I recall from the KIP, the ELR is normally empty, so maybe having this parallel data structure isn't so bad (since it will normally not be populated)?
@cmccabe since you worked on
BrokersToIsrsoriginally, do you have any opinion here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like that in order to work correctly, we need to make sure that ISR and ELR are always disjoint sets (otherwise we'd generate multiple records for the same partition). Can we add a comment about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the comment in the ReplicationControlManager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we keeping this as a second data structure and we need to ensure BrokersToIsrs and BrokersToElrs are disjoint, I think we need some guards/checks. For example, it seems we are always updating both data structures
If we need to ensure a replica is not in the ISR and ELR for correctness, we should have a check.