KAFKA-20110 Fix DynamicQuotaConfig

[see-quick]

This PR fixes a problem with a few broker properties. More can be found https://issues.apache.org/jira/browse/KAFKA-20110.

[chia7712]

is this duplicate to #21131?

[see-quick]

is this duplicate to #21131?

Hmmm, partially (as it fixes the isReadOnly)? However, my PR also adds to PER_BROKER_CONFIGS and AbstractKafkaConfig.CONFIG_DEFto display it on the Kafka site within the broker [1] (currently it's not there).

Does #21131 fix the website issue? 🤔

[1] - https://kafka.apache.org/41/configuration/

[chia7712]

A new ConfigDef is required for each call because the one from brokerQuotaConfigs is subject to modification

[see-quick]

Hmm, I think problem is more deeper ... In the, we can see

kafka/server/src/main/java/org/apache/kafka/server/config/DynamicConfig.java

Lines 36 to 43 in 9d5bbf5

	ConfigDef configs = QuotaConfig.brokerQuotaConfigs();
	// Filter and define all dynamic configurations
	AbstractKafkaConfig.CONFIG_DEF.configKeys().forEach((name, value) -> {
	if (DynamicBrokerConfig.ALL_DYNAMIC_CONFIGS.contains(name)) {
	configs.define(value);
	}
	});
	BROKER_CONFIGS = configs;

I don't why there is QuotaConfig.brokerQuotaConfigs(); as other configs is loaded from ALL_DYNAMIC_CONFIGS. Maybe that's because someone forgot to include it inside the ALL_DYNAMIC_CONFIGS backthen...

Anyway, I have removed/replaced ConfigDef configs = QuotaConfig.brokerQuotaConfigs() as it is already included and defined in the ALL_DYNAMIC_CONFIGS, and it seems to work. Also checked the website and showing the right values.

Let me know what you think.

[chia7712]

Maybe that's because someone forgot to include it inside the ALL_DYNAMIC_CONFIGS backthen...

yes, that's indeed an issue, and #21131 tries to fix it :)

[see-quick]

Yeah, but still this line of code.

kafka/server/src/main/java/org/apache/kafka/server/config/DynamicConfig.java

Line 36 in 9d5bbf5

ConfigDef configs = QuotaConfig.brokerQuotaConfigs();

I don't get why we have just 3 properties, which are configured only dynamically. Why not also dual-mode (i.e., static || dynamic)? Like for instance num.io.threads works fine in dual-mode but probably I should read a KIP, which introduced those properties... but it's bit weird.

At least I think we should add comment here that in such line DynamicConfig.java#L36 those configs are dynamic-only and not dual-mode as others.

[chia7712]

I don't get why we have just 3 properties, which are configured only dynamically. Why not also dual-mode (i.e., static || dynamic)? Like for instance num.io.threads works fine in dual-mode but probably I should read a KIP, which introduced those properties... but it's bit weird.

You might want to check out the KIP-1051, which address the exact issue

At least I think we should add comment here that in such line DynamicConfig.java#L36 those configs are dynamic-only and not dual-mode as others.

Actually, the comment is already there. see https://github.com/apache/kafka/blob/trunk/server/src/main/java/org/apache/kafka/server/config/DynamicConfig.java#L27

[see-quick]

You might want to check out the KIP-1051, which address the exact issue

So it's still under-discussion? I am bit a lost in the JIRAs like there is always at least one JIRA for issue I found :D . Because that KIP just wants to move such configuration from DynamicConfig to KafkaConfig which is not a case.

Actually, the comment is already there. see https://github.com/apache/kafka/blob/trunk/server/src/main/java/org/apache/kafka/server/config/DynamicConfig.java#L27

Class used to hold dynamic configs. These are configs which have no physical manifestation in the server.properties and can only be set dynamically.

used to (past)? It's still holding dynamic-only configs (i.e., QuotaConfig.brokerQuotaConfigs();)? And then you have also dual-mode configs which are filtered from the AbstractKafkaConfig i.e., :

// Filter and define all dynamic configurations
            AbstractKafkaConfig.CONFIG_DEF.configKeys().forEach((name, value) -> {
                if (DynamicBrokerConfig.ALL_DYNAMIC_CONFIGS.contains(name)) {
                    configs.define(value);
                }
            });

I would probably fix that doc something like:

Holds dynamic configs, including both dynamic-only configs (e.g., quotas)                                                                                                                                                           
and dual-mode configs that can be set statically or dynamically.    

I think it would help with overall understanding.

[chia7712]

These should not be added to AbstractKafkaConfig.CONFIG_DEF, as they cannot be set in server.properties

see https://issues.apache.org/jira/browse/KAFKA-20125

[see-quick]

Okay, so basically if I get it right. If property can't be configured in server.properties (i.e., static broker configs). For some dynamic-only (i.e., leader.replication.throttled.rate ... ) configs they won't be in the section [1]? So that's basically correct?

[1] - https://kafka.apache.org/41/configuration/broker-configs/

[see-quick]

FYI @showuon. If my assumption is right.

[chia7712]

they won't be in the section [1]? So that's basically correct?

I think it's still beneficial to document them. The updates in KAFKA-20125 will remind users that these configs are dynamic-only. We could modify the config generation logic instead of adding them to AbstractKafkaConfig.CONFIG_DEF.