KAFKA-17584: Fix incorrect synonym handling for dynamic log configurations by cmccabe · Pull Request #17258 · apache/kafka

cmccabe · 2024-09-23T23:14:02Z

Several Kafka log configurations in have synonyms. For example, log retention can be configured either by log.retention.ms, or by log.retention.minutes, or by log.retention.hours. There is also a faculty in Kafka to dynamically change broker configurations without restarting the broker. These dynamically set configurations are stored in the metadata log and override what is in the broker properties file.

Unfortunately, these two features interacted poorly; there was a bug where the dynamic log configuration update code ignored synonyms. For example, if you set log.retention.minutes and then reconfigured something unrelated that triggered the LogConfig update path, the retention value that you had configured was overwritten.

The reason for this was incorrect handling of synonyms. The code tried to treat the Kafka broker configuration as a bag of key/value entities rather than extracting the correct retention time (or other setting with overrides) from the KafkaConfig object.

…tions Several Kafka log configurations in have synonyms. For example, log retention can be configured either by log.retention.ms, or by log.retention.minutes, or by log.retention.hours. There is also a faculty in Kafka to dynamically change broker configurations without restarting the broker. These dynamically set configurations are stored in the metadata log and override what is in the broker properties file. Unfortunately, these two features interacted poorly; there was a bug where the dynamic log configuration update code ignored synonyms. For example, if you set log.retention.minutes and then reconfigured something unrelated that triggered the LogConfig update path, the retention value that you had configured was overwritten. The reason for this was incorrect handling of synonyms. The code tried to treat the Kafka broker configuration as a bag of key/value entities rather than extracting the correct retention time (or other setting with overrides) from the KafkaConfig object. Separately from the above bug, the code did not honor the value of dynamically configured synonyms: setting log.retention.minutes had no effect; only log.retention.ms was honored.

showuon

LGTM! Minor comment left.
There's a DynamicBrokerReconfigurationTest.testConfigDescribeUsingAdminClient test failed. I had a look, it looks like we were validating the wrong thing before. The log.retention.hours should be not a read-only config. It can be dynamically changed, as well as log.roll.hours.

showuon · 2024-09-24T06:29:10Z

+    results.asScala
+  }
+
+  val KafkaConfigToLogConfigName: Map[String, String] = {


nit: After this PR, this variable is only used in DynamicBrokerReconfigurationTest. We can move it there.

fvaleri · 2024-09-24T08:41:54Z

The log.retention.hours should be not a read-only config. It can be dynamically changed, as well as log.roll.hours.

I think we should also update the documentation, as they are both reported as read-only.

jayteej · 2024-09-24T09:00:02Z

-    ServerTopicConfigSynonyms.TOPIC_CONFIG_SYNONYMS.values.asScala.toSet - ServerLogConfigs.LOG_MESSAGE_FORMAT_VERSION_CONFIG
-  val KafkaConfigToLogConfigName: Map[String, String] =
-    ServerTopicConfigSynonyms.TOPIC_CONFIG_SYNONYMS.asScala.map { case (k, v) => (v, k) }
+  val ReconfigurableConfigs: Set[String] = {


nit (immutable style):

val ReconfigurableConfigs: Set[String] = { ServerTopicConfigSynonyms.ALL_TOPIC_CONFIG_SYNONYMS .values() .asScala .flatMap(v => v.asScala.map(configSynonym => configSynonym.name())) .filterNot(_ == ServerLogConfigs.LOG_MESSAGE_FORMAT_VERSION_CONFIG) .toSet }

showuon · 2024-09-24T09:00:24Z

The log.retention.hours should be not a read-only config. It can be dynamically changed, as well as log.roll.hours.

I think we should also update the documentation, as they are both reported as read-only.

Good point! But the doc is actually generated, and I just confirmed, after this PR, the doc will be correctly updated:

log.retention.hours

The number of hours to keep a log file before deleting it (in hours), tertiary to log.retention.ms property
Type:	int
Default:	168
Valid Values:	
Importance:	high
Update Mode:	cluster-wide
log.retention.minutes

The number of minutes to keep a log file before deleting it (in minutes), secondary to log.retention.ms property. If not set, the value in log.retention.hours is used
Type:	int
Default:	null
Valid Values:	
Importance:	high
Update Mode:	cluster-wide
log.retention.ms

The number of milliseconds to keep a log file before deleting it (in milliseconds), If not set, the value in log.retention.minutes is used. If set to -1, no time limit is applied.
Type:	long
Default:	null
Valid Values:	
Importance:	high
Update Mode:	cluster-wide
log.roll.hours

The maximum time before a new log segment is rolled out (in hours), secondary to log.roll.ms property
Type:	int
Default:	168
Valid Values:	[1,...]
Importance:	high
Update Mode:	cluster-wide
log.roll.jitter.hours

The maximum jitter to subtract from logRollTimeMillis (in hours), secondary to log.roll.jitter.ms property
Type:	int
Default:	0
Valid Values:	[0,...]
Importance:	high
Update Mode:	cluster-wide

clolov

The log.retention.hours should be not a read-only config. It can be dynamically changed, as well as log.roll.hours.

I think we should also update the documentation, as they are both reported as read-only.

Good point! But the doc is actually generated, and I just confirmed, after this PR, the doc will be correctly updated

My only call out is that currently (in trunk) log.retention.hours/minutes are documented as read-only and behave as read-only. After this change they become cluster-wide in both documentation and behaviour. I am happy with this change as long as we publicly call it out in some form.

fvaleri · 2024-09-24T09:28:45Z

we publicly call it out in some form

Agreed. This is what happens right now, so it would be a behavior change. I think a simple release note would be fine.

$ bin/kafka-configs.sh --bootstrap-server :9092 --entity-type brokers --entity-name 2 --alter --add-config log.retention.minutes=1
Error while executing config command with args '--bootstrap-server :9092 --entity-type brokers --entity-name 2 --alter --add-config log.retention.minutes=1'
java.util.concurrent.ExecutionException: org.apache.kafka.common.errors.InvalidRequestException: Cannot update these configs dynamically: Set(log.retention.minutes)
	at java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:396)
	at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2096)
	at org.apache.kafka.common.internals.KafkaFutureImpl.get(KafkaFutureImpl.java:180)
	at kafka.admin.ConfigCommand$.alterConfig(ConfigCommand.scala:390)
	at kafka.admin.ConfigCommand$.processCommand(ConfigCommand.scala:351)
	at kafka.admin.ConfigCommand$.main(ConfigCommand.scala:100)
	at kafka.admin.ConfigCommand.main(ConfigCommand.scala)
Caused by: org.apache.kafka.common.errors.InvalidRequestException: Cannot update these configs dynamically: Set(log.retention.minutes)

showuon · 2024-09-24T09:55:06Z

I've sent a mail dev/user mailing list to discuss it here. FYI.

fvaleri

Thanks @showuon.

mumrah · 2024-09-24T13:23:47Z

Looks like there is a failing test:

DynamicBrokerReconfigurationTest > "testConfigDescribeUsingAdminClient(String)

… configs

cmccabe · 2024-09-24T18:02:07Z

For now, I'm going to change it back to the current behavior where we ignore the value of dynamically configured synonyms such as log.retention.minutes, etc. We can make that change in a separate PR if people feel strongly about it. (The fact that it's been like this since 3.0 seems like a good reason to leave it for now....)

showuon

LGTM! It's fine we keep the old behavior since it won't block any operation.

kamalcph · 2024-09-25T05:25:03Z

This issue was first reported in KAFKA-15266 and a PR #14119 is opened for it.

amangandhi94

This looks good to me and addresses the bug I had raised - https://issues.apache.org/jira/browse/KAFKA-15266

Just wondering how can I get more attention to bugs or PR i have raised next time onwards?

amangandhi94 · 2024-09-25T06:45:27Z

  override def reconfigure(oldConfig: KafkaConfig, newConfig: KafkaConfig): Unit = {
    val originalLogConfig = logManager.currentDefaultConfig
    val originalUncleanLeaderElectionEnable = originalLogConfig.uncleanLeaderElectionEnable
-    val newBrokerDefaults = new util.HashMap[String, Object](originalLogConfig.originals)


Just curious - Do we know why this logic was even added? I cant seem to understand if there was any benefit to doing this.

See discussion here #17258 (comment)

fvaleri · 2024-09-25T07:21:18Z

Just wondering how can I get more attention to bugs or PR i have raised next time onwards?

In this case raising the bug as a blocker, due to the potential data loss. In general, sending an email to the dev mailing list would be enough to catch some more attention in my experience. Hope it helps.

fvaleri

LGTM. Thanks.

kamalcph

LGTM

rajinisivaram · 2024-09-25T10:58:10Z

    val originalUncleanLeaderElectionEnable = originalLogConfig.uncleanLeaderElectionEnable
-    val newBrokerDefaults = new util.HashMap[String, Object](originalLogConfig.originals)
-    newConfig.valuesFromThisConfig.forEach { (k, v) =>
-      if (DynamicLogConfig.ReconfigurableConfigs.contains(k)) {


It has been a long time, so I may not remember correctly. But I think we used to only update configs which are ReconfigurableConfigs even if you updated ZooKeeper directly with new configs that included others. Non-reconfigurable configs would be picked up only on the next broker restart. Do we think it is safe to include all configs now either because we don't allow other configs to be updated in KRaft or because all log configs are reconfigurable now?

Thanks for this comment, @rajinisivaram . I think you're right that we should be excluding changes to configurations that don't appear in DynamicLogConfig.ReconfigurableConfigs. I have added some code to do this now. I think from a practical point of view, this only affects message.format.version, which we explicitly excluded from being dynamically reconfigurable.

Do we think it is safe to include all configs now either because we don't allow other configs to be updated in KRaft or because all log configs are reconfigurable now?

KRaft has the same behavior as ZK, in that it lets you set any broker configuration you want, whether it's valid or not. There were too many people depending on this for us to change it in 3.x. Maybe in the future if someone creates a KIP...

Of course the command-line tool does its own validation.

Good call @rajinisivaram !

…urable - Add testLogRetentionTimeMinutesIsNotDynamicallyReconfigurable - clean up some cases where we were using zkconnect but did not need to

rajinisivaram

@cmccabe Thanks for the PR, LGTM

junrao

@cmccabe : Thanks for the PR. Added a couple of comments.

junrao · 2024-09-25T21:02:25Z

+    val newBrokerDefaults = new util.HashMap[String, Object](newConfig.extractLogConfigMap)
+    originalLogConfig.originals().forEach((k, v) => {
+      if (!DynamicLogConfig.ReconfigurableConfigs.contains(k)) {
+        newBrokerDefaults.put(k, v)


Do we need to translate the name from KafkaConfig to LogConfig as the original code does?

We started newBrokerDefaults with newConfig.extractLogConfigMap that includes non-reconfigurable configs. Where is the logic to remove them?

Let me fix this a bit. It should be sufficient to just remove the non-reconfigurable configs (really, singular config)

…log config

junrao

@cmccabe : Thanks for the updated PR. A few more comments.

junrao · 2024-09-25T23:10:38Z

+    val newBrokerDefaults = new util.HashMap[String, Object](newConfig.extractLogConfigMap)
+    val originalLogConfigMap = originalLogConfig.originals()
+    DynamicLogConfig.NonReconfigrableLogConfigs.foreach(k => {
+      Option(originalLogConfigMap.get(k)) match {


Hmm, originalLogConfigMap is the current config. We should check from newConfig, right?

Non-reconfigurable configs are copied over from the current (not new) configuration.

junrao · 2024-09-25T23:11:47Z

+    DynamicLogConfig.NonReconfigrableLogConfigs.foreach(k => {
+      Option(originalLogConfigMap.get(k)) match {
+        case None => newBrokerDefaults.remove(k)
+        case Some(v) => newBrokerDefaults.put(k, v)


Not sure that I follow here. Why are we putting a non-reconfigurable config to newBrokerDefaults?

Because we want the non-reconfigurable configuration to have the same value (or lack of value) that it had previously. It should not change.

junrao · 2024-09-25T23:13:10Z

+        case Some(v) => newBrokerDefaults.put(k, v)
      }
-    }
+    })


newBrokerDefaults has the config name with log prefix, right? Should we translate the name from KafkaConfig to LogConfig as the original code does?

newBrokerDefaults has the config name with log prefix, right

no. it comes from KafkaConfig.extractLogConfigMap, which creates a map of log (aka topic) configurations, not broker configurations.

junrao

@cmccabe : Thanks for the explanation. LGTM

showuon · 2024-09-26T06:04:54Z

All tests passed.

…tions (#17258) Several Kafka log configurations in have synonyms. For example, log retention can be configured either by log.retention.ms, or by log.retention.minutes, or by log.retention.hours. There is also a faculty in Kafka to dynamically change broker configurations without restarting the broker. These dynamically set configurations are stored in the metadata log and override what is in the broker properties file. Unfortunately, these two features interacted poorly; there was a bug where the dynamic log configuration update code ignored synonyms. For example, if you set log.retention.minutes and then reconfigured something unrelated that triggered the LogConfig update path, the retention value that you had configured was overwritten. The reason for this was incorrect handling of synonyms. The code tried to treat the Kafka broker configuration as a bag of key/value entities rather than extracting the correct retention time (or other setting with overrides) from the KafkaConfig object. Reviewers: Luke Chen <showuon@gmail.com>, Jun Rao <junrao@gmail.com>, Kamal Chandraprakash<kamal.chandraprakash@gmail.com>, Christo Lolov <lolovc@amazon.com>, Federico Valeri <fedevaleri@gmail.com>, Rajini Sivaram <rajinisivaram@googlemail.com>, amangandhi94 <>

…tions (apache#17258) Several Kafka log configurations in have synonyms. For example, log retention can be configured either by log.retention.ms, or by log.retention.minutes, or by log.retention.hours. There is also a faculty in Kafka to dynamically change broker configurations without restarting the broker. These dynamically set configurations are stored in the metadata log and override what is in the broker properties file. Unfortunately, these two features interacted poorly; there was a bug where the dynamic log configuration update code ignored synonyms. For example, if you set log.retention.minutes and then reconfigured something unrelated that triggered the LogConfig update path, the retention value that you had configured was overwritten. The reason for this was incorrect handling of synonyms. The code tried to treat the Kafka broker configuration as a bag of key/value entities rather than extracting the correct retention time (or other setting with overrides) from the KafkaConfig object. Reviewers: Luke Chen <showuon@gmail.com>, Jun Rao <junrao@gmail.com>, Kamal Chandraprakash<kamal.chandraprakash@gmail.com>, Christo Lolov <lolovc@amazon.com>, Federico Valeri <fedevaleri@gmail.com>, Rajini Sivaram <rajinisivaram@googlemail.com>, amangandhi94 <>

…tions (#17258) (#17278) Several Kafka log configurations in have synonyms. For example, log retention can be configured either by log.retention.ms, or by log.retention.minutes, or by log.retention.hours. There is also a faculty in Kafka to dynamically change broker configurations without restarting the broker. These dynamically set configurations are stored in the metadata log and override what is in the broker properties file. Unfortunately, these two features interacted poorly; there was a bug where the dynamic log configuration update code ignored synonyms. For example, if you set log.retention.minutes and then reconfigured something unrelated that triggered the LogConfig update path, the retention value that you had configured was overwritten. The reason for this was incorrect handling of synonyms. The code tried to treat the Kafka broker configuration as a bag of key/value entities rather than extracting the correct retention time (or other setting with overrides) from the KafkaConfig object. Reviewers: Luke Chen <showuon@gmail.com>, Jun Rao <junrao@gmail.com>, Kamal Chandraprakash<kamal.chandraprakash@gmail.com>, Christo Lolov <lolovc@amazon.com>, Federico Valeri <fedevaleri@gmail.com>, Rajini Sivaram <rajinisivaram@googlemail.com>, amangandhi94 <> Co-authored-by: Colin Patrick McCabe <cmccabe@apache.org> Reviewers: Josep Prat <josep.prat@aiven.io>

…tions This is a cherry-pick of #17258 to 3.7.2 This commit differs from the original by using the old (read 3.7) references to the configurations and not changing as many unit tests Reviewers: Divij Vaidya <diviv@amazon.com>, Colin Patrick McCabe <cmccabe@apache.org>

…tions (apache#17258) Several Kafka log configurations in have synonyms. For example, log retention can be configured either by log.retention.ms, or by log.retention.minutes, or by log.retention.hours. There is also a faculty in Kafka to dynamically change broker configurations without restarting the broker. These dynamically set configurations are stored in the metadata log and override what is in the broker properties file. Unfortunately, these two features interacted poorly; there was a bug where the dynamic log configuration update code ignored synonyms. For example, if you set log.retention.minutes and then reconfigured something unrelated that triggered the LogConfig update path, the retention value that you had configured was overwritten. The reason for this was incorrect handling of synonyms. The code tried to treat the Kafka broker configuration as a bag of key/value entities rather than extracting the correct retention time (or other setting with overrides) from the KafkaConfig object. Reviewers: Luke Chen <showuon@gmail.com>, Jun Rao <junrao@gmail.com>, Kamal Chandraprakash<kamal.chandraprakash@gmail.com>, Christo Lolov <lolovc@amazon.com>, Federico Valeri <fedevaleri@gmail.com>, Rajini Sivaram <rajinisivaram@googlemail.com>, amangandhi94 <>

github-actions Bot added the core Kafka Broker label Sep 23, 2024

cmccabe force-pushed the KAFKA-17584 branch from b182d44 to 8cf31e0 Compare September 23, 2024 23:16

showuon self-assigned this Sep 24, 2024

showuon approved these changes Sep 24, 2024

View reviewed changes

jayteej reviewed Sep 24, 2024

View reviewed changes

Comment thread core/src/main/scala/kafka/server/DynamicBrokerConfig.scala Outdated

clolov approved these changes Sep 24, 2024

View reviewed changes

showuon assigned fvaleri Sep 24, 2024

fvaleri reviewed Sep 24, 2024

View reviewed changes

Go back to the old behavior where synonyms cannot be used for dynamic…

4ce9b0d

… configs

remove testDynamicLogReconfigurableConfigsIncludesDeprecatedSynonyms

297e961

showuon approved these changes Sep 25, 2024

View reviewed changes

kamalcph self-requested a review September 25, 2024 05:30

showuon mentioned this pull request Sep 25, 2024

KAFKA-15266: Static configs set for non primary synonyms are ignored for Log configs #14119

Closed

3 tasks

fvaleri removed their assignment Sep 25, 2024

amangandhi94 approved these changes Sep 25, 2024

View reviewed changes

kamalcph removed their request for review September 25, 2024 07:00

fvaleri approved these changes Sep 25, 2024

View reviewed changes

kamalcph approved these changes Sep 25, 2024

View reviewed changes

rajinisivaram reviewed Sep 25, 2024

View reviewed changes

Do not allow log configurations to be changed if they're not reconfig…

0a2752a

…urable - Add testLogRetentionTimeMinutesIsNotDynamicallyReconfigurable - clean up some cases where we were using zkconnect but did not need to

rajinisivaram approved these changes Sep 25, 2024

View reviewed changes

junrao reviewed Sep 25, 2024

View reviewed changes

rework the reconfiguration logic a bit to fix the non-reconfigurable …

d22d58d

…log config

junrao reviewed Sep 25, 2024

View reviewed changes

junrao approved these changes Sep 26, 2024

View reviewed changes

showuon merged commit 3d23029 into trunk Sep 26, 2024

showuon mentioned this pull request Sep 26, 2024

KAFKA-17584: Fix incorrect synonym handling for dynamic log configurations #17278

Merged

3 tasks

clolov mentioned this pull request Nov 5, 2024

KAFKA-17584: Fix incorrect synonym handling for dynamic log configurations #17696

Merged

Conversation

cmccabe commented Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

showuon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fvaleri commented Sep 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

showuon commented Sep 24, 2024

Uh oh!

Uh oh!

clolov left a comment

Choose a reason for hiding this comment

Uh oh!

fvaleri commented Sep 24, 2024

Uh oh!

showuon commented Sep 24, 2024

Uh oh!

fvaleri left a comment

Choose a reason for hiding this comment

Uh oh!

mumrah commented Sep 24, 2024

Uh oh!

cmccabe commented Sep 24, 2024

Uh oh!

showuon left a comment

Choose a reason for hiding this comment

Uh oh!

kamalcph commented Sep 25, 2024

Uh oh!

amangandhi94 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!

fvaleri commented Sep 25, 2024

Uh oh!

fvaleri left a comment

Choose a reason for hiding this comment

Uh oh!

kamalcph 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!

rajinisivaram left a comment

Choose a reason for hiding this comment

Uh oh!

junrao 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!

junrao 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

cmccabe commented Sep 23, 2024 •

edited

Loading