Kafka-16540: Update partitions if min isr config is changed. by CalvinLiu7947 · Pull Request #15702 · apache/kafka

CalvinLiu7947 · 2024-04-12T00:59:16Z

https://issues.apache.org/jira/browse/KAFKA-16540
If the min isr config is changed, we need to update the partitions with ELR if possible.

CalvinLiu7947 · 2024-04-12T16:01:51Z

@mumrah Can you help take a look?

splett2 · 2024-04-12T21:05:35Z

+    void maybeTriggerMinIsrConfigUpdate(Optional<String> topicName) throws InterruptedException, ExecutionException {
+        appendWriteEvent("partitionUpdateForMinIsrChange", OptionalLong.empty(),
+            () -> replicationControl.getPartitionElrUpdatesForConfigChanges(topicName)).get();
+    }


calling .get() on an appendWriteEvent doesn't look right to me. If I understand correctly, the appendWriteEvents are handled in the quorum controller event loop thread.

We would expect replay() to also be called in the event loop thread. so if we trigger an appendWriteEvent and block waiting for the result, it would always time out, since we are blocking the processing thread.

Got it, we basically only need to call the appendWriteEvents and do not wait for the replay().

hmm, I think a better way to think about it is that we want to append the min ISR config update atomically with the partition change records. Appending the partition change records once the config change is replayed is difficult to reason about and possibly incorrect. Thinking a bit more about it, triggering a write event from the replay() for the config change record means that every time we reload the metadata log, we would replay the config change record and generate new partition change records.

Perhaps one example to look at is ReplicationControlManager.handleBrokerFenced. When a broker is fenced, we generate a broker registration change record along with the leaderAndIsr partition change records. I assume we want to follow a similar model with the topic configuration change events.

Make sense, I have some misunderstanding about the controller events. Will update. Thanks!

splett2 · 2024-04-25T17:41:42Z

+                if (configRecord.name().equals(TopicConfig.MIN_IN_SYNC_REPLICAS_CONFIG)) {
+                    minIsrRecords.add(configRecord);
+                    if (Type.forId(configRecord.resourceType()) == Type.TOPIC) {
+                        if (configRecord.value() == null) topicMap.put(configRecord.resourceName(), configRecord.value());
+                        else configRemovedTopicMap.put(configRecord.resourceName(), configRecord.value());
+                    }
+                }
+            }
+        });


what is the behavior if the default broker config for min.insync.replicas is changed?
I am not actually sure how that impacts the min.insync.replicas for existing topics.

If min.insync.replicas is not set on the topic config level, the effective min.insync.replicas of a topic will change if default broker config is updated.

Yeah, unfortunately we have a 5 level system:

topic configs (highest priority)

node configs (aka "broker" configs, but they also apply to controllers)

cluster configs (aka default configs for the broker resource)

static configuration

static default

The first three levels can change at runtime 🤮

splett2 · 2024-04-25T17:43:39Z

+        for (ConfigRecord record : minIsrRecords) {
+            replayInternal(record, configDataCopy, localSnapshotRegistry);
+        }


why are we calling replay here?

This is the implementation challenge part of this PR. To find the effective min ISR value, it requires checking topic config -> dynamic broker config -> default broker config -> ...
Let's say the user updates the default broker config:

All the topics could be affected.

The effective min ISR values should be recalculated.

We need to generate the partition change records along with the config change records, which means the ReplicationControlManager can't use the regular methods for the effective min ISR value. The value should be determined by the config records and the current configs.

I found it easier to make a copy of the configs and apply the min ISR updates on the copy. Then let the ReplicationControlManager check all the partitions with the config copy.

splett2 · 2024-04-25T17:49:28Z

    private final TimelineHashMap<ConfigResource, TimelineHashMap<String, String>> configData;
    private final Map<String, Object> staticConfig;
    private final ConfigResource currentController;
+    private final MinIsrConfigUpdatePartitionHandler minIsrConfigUpdatePartitionHandler;


maybe more of a question for someone with more code ownership of the quorum controller code, but I wonder if it would be preferable to handle generating the replication control manager records in the QuorumController.incrementalAlterConfigs. That would also make it a bit easier to handle validateOnly which we are not currently handling.

cmccabe · 2024-05-24T23:49:04Z

+        // Because it may require multiple layer look up for the min ISR config value. Build a config data copy and apply
+        // the config updates to it. Use this config copy for the min ISR look up.
+        Map<ConfigResource, TimelineHashMap<String, String>> pendingConfigData = new HashMap<>();
+        SnapshotRegistry localSnapshotRegistry = new SnapshotRegistry(new LogContext("dummy-config-update"));


Maybe I'm missing something, but I don't think this works... pendingConfigData will have some of the new changes you made, but not all of the existing changes. So, for example, perhaps we are changing the cluster config for min topic ISR, but the node config for the current controller node is unchanged. It should take priority, but it won't be in here.

Yeah, it is not a straightforward change. So the pending config data populated here will be checked together with the existing configs. See how OrderedConfigResolver is used below.

cmccabe · 2024-05-24T23:50:40Z

+    String getTopicConfigWithPendingChange(
+        String topicName,
+        String configKey,
+        Map<ConfigResource, TimelineHashMap<String, String>> pendingConfigData


It seems like there could be some value in turning the pending config change into a class of its own, if we're going to be querying it like this.

I used a OrderedConfigResolver later to solve the problem that we have to look up both pending config data and current config.

cmccabe · 2024-05-24T23:52:42Z

        return ApiError.NONE;
    }

+    void maybeTriggerPartitionUpdateOnMinIsrChange(List<ApiMessageAndVersion> records) {


I guess eventually we will probably want some more general system for responding to configuration changes, and not special-case min.isr. However, we don't have to do that in this PR.

I also understand why you had this function receive a list of records here rather than something fancier (easier to integrate into the existing code, and into the two configuration change paths.)

mumrah

Thanks for the patch, @CalvinConfluent! Left some initial feedback

mumrah · 2024-07-09T18:40:43Z

        return ApiError.NONE;
    }

+    void maybeTriggerPartitionUpdateOnMinIsrChange(List<ApiMessageAndVersion> records) {


The complexity here is a tad bit high. Can we extract a method for getting the minIsrRecords ConfigRecord from List<ApiMessageAndVersion>?

mumrah · 2024-07-09T19:14:00Z

+        });
+
+        if (minIsrRecords.isEmpty()) return;
+        if (topicToMinIsrValueMap.size() == minIsrRecords.size()) {


The size comparison here and below are a little non-obvious (to me at least). Maybe we can set a boolean as we're looping through the records to determine if we hit this branch.

Alternative question, is this optimization helping with performance? We still need the code for the case of overlaying configs from different levels, so having this separate code path just increases complexity.

mumrah · 2024-07-09T19:18:04Z

+                        if (configRecord.value() != null) topicToMinIsrValueMap.put(configRecord.resourceName(), configRecord.value());
+                        else configRemovedTopicMap.put(configRecord.resourceName(), configRecord.value());


style nit: Can you reformat these to not be inline?

mumrah · 2024-07-09T19:28:32Z

+        }
+
+        ArrayList<String> topicList = new ArrayList<>();
+        // If all the updates are on the Topic level, we can avoid perform a full scan of the partitions.


Similar to the above comment, this size check is hard to grok. Maybe we can compute a boolean to determine if we need to inspect every partition vs just a subset.

In what case do we need to scan all partitions? Only when the cluster-level min.insync.replicas is changed?

mumrah · 2024-07-10T16:01:35Z

+     * @param record                 The ConfigRecord.
+     * @param localConfigData        The config data is going to be updated.
+     */
+    public void replayForPendingConfig(


Can we reuse ConfigurationsImage here instead of adding another place where we are applying records? I think it should be reasonably straightforward to construct a ConfigurationsImage with the in-memory state (localConfigData) and then replay records to get a ConfigurationsDelta.

mumrah · 2024-07-10T17:31:36Z

+            configs = new ArrayList<>();
+            configs.add(map);
+        }
+        public boolean containsKey(String key) {


style nit: whitespace before method signature

docs: can you document this method's behavior here?

mumrah · 2024-07-10T17:33:03Z

+        }
+        public boolean containsKey(String key) {
+            for (Map<String, ?> config : configs) {
+                if (config.containsKey(key)) return config.get(key) != null;


If a later config map has a non-null, I think we could mistakenly return false here.

mumrah · 2024-07-10T17:53:57Z

+        Map<String, String> pendingControllerConfig =
+            pendingConfigData.containsKey(currentController) ? pendingConfigData.get(currentController) : Collections.emptyMap();
+        return configSchema.resolveEffectiveTopicConfigs(
+            new OrderedConfigResolver(staticConfig),


Hm.. what happens if the controller has different static configs from the broker?

mumrah · 2024-07-10T18:44:31Z

        int currentMinIsr = defaultMinIsr;
-        String minIsrConfig = configurationControl.getTopicConfig(topicName, MIN_IN_SYNC_REPLICAS_CONFIG);
+        String minIsrConfig;
+        if (getTopicMinIsrConfig == null) {


Can we split the method into two signatures instead of having the null?

E.g., int getTopicEffectiveMinIsr(String topicName) and int getTopicEffectiveMinIsr(String topicName, Function<String, String> getTopicMinIsrConfig)

cmccabe · 2024-11-12T22:03:14Z

        return new BrokersToIsrs.PartitionsOnReplicaIterator(topicMap, false);
    }
+
+    BrokersToIsrs.PartitionsOnReplicaIterator partitionsWithElr() {


This isn't acceptable from a performance point of view, since it will be O(num_partitions)

CalvinLiu7947 · 2024-12-11T22:07:45Z

Close this PR for #18148 because there are too many things since this PR was opened.

Kafka-16540: Update partitions if min isr config is changed.

e6db700

splett2 reviewed Apr 12, 2024

View reviewed changes

Comment thread metadata/src/main/java/org/apache/kafka/controller/ConfigurationControlManager.java Outdated

splett2 reviewed Apr 12, 2024

View reviewed changes

Comment thread metadata/src/main/java/org/apache/kafka/controller/ConfigurationControlManager.java Outdated

splett2 reviewed Apr 12, 2024

View reviewed changes

Do not wait for the write event to be executed

8cda28a

CalvinLiu7947 requested a review from splett2 April 15, 2024 21:44

Minor

13ee2a7

splett2 reviewed Apr 17, 2024

View reviewed changes

Comment thread metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java Outdated

CalvinLiu7947 added 2 commits April 17, 2024 16:00

Generate the partition records with the config records

9615930

Add more tests

3a232a9

CalvinLiu7947 requested a review from splett2 April 17, 2024 23:34

splett2 reviewed Apr 25, 2024

View reviewed changes

Comment thread metadata/src/main/java/org/apache/kafka/controller/BrokersToElrs.java Outdated

splett2 reviewed Apr 25, 2024

View reviewed changes

Comment thread metadata/src/main/java/org/apache/kafka/controller/ConfigurationControlManager.java

splett2 reviewed Apr 25, 2024

View reviewed changes

CalvinLiu7947 added 2 commits April 29, 2024 10:01

Testing legacyAlterConfig

bbc9956

Using a ordered resolver to get the pending config

f2fec49

CalvinLiu7947 requested a review from splett2 May 9, 2024 00:15

cmccabe reviewed May 24, 2024

View reviewed changes

Fix the bug for config removal

bebdaf0

CalvinLiu7947 requested a review from cmccabe May 28, 2024 23:16

CalvinLiu7947 mentioned this pull request May 31, 2024

KAFKA-16480: Bump ListOffsets version, IBP version and mark last version of ListOffsets as unstable #15673

Closed

Merge branch 'trunk' into ELR-config-update-2

df04f30

mumrah reviewed Jul 10, 2024

View reviewed changes

cmccabe reviewed Nov 12, 2024

View reviewed changes

CalvinLiu7947 closed this Dec 11, 2024

		if (configRecord.value() != null) topicToMinIsrValueMap.put(configRecord.resourceName(), configRecord.value());
		else configRemovedTopicMap.put(configRecord.resourceName(), configRecord.value());

Conversation

CalvinLiu7947 commented Apr 12, 2024

Uh oh!

CalvinLiu7947 commented Apr 12, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

splett2 Apr 16, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mumrah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CalvinLiu7947 commented Dec 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

splett2 Apr 16, 2024 •

edited

Loading