KAFKA-16938: non-dynamic props gets corrupted due to circular reference between DynamicBrokerConfig and DynamicConfig. by frankvicky · Pull Request #16302 · apache/kafka

frankvicky · 2024-06-12T09:39:29Z

The circular reference between DynamicBrokerConfig and DynamicConfig results in unexpected outcomes when initializing nonDynamicProps. The solution is to delegate the initialization responsibility to DynamicConfig to avoid the circular reference.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

… DynamicConfig

chia7712

@frankvicky thanks for updated PR.

chia7712 · 2024-06-12T09:54:51Z

-  object Broker {
-    private val brokerConfigs = QuotaConfigs.brokerQuotaConfigs()
-    DynamicBrokerConfig.addDynamicConfigs(brokerConfigs)
+      val nonDynamicProps: Set[String] = KafkaConfig.configNames.toSet -- brokerConfigs.names.asScala


Does it have correct indentation?

Oops, I will fix it.

chia7712 · 2024-06-12T09:56:23Z

+      private val brokerConfigs = {
+        val configs = QuotaConfigs.brokerQuotaConfigs()
+        KafkaConfig.configKeys
+          .filter { case (configName, _) => AllDynamicConfigs.contains(configName) }


Could you please add comments to remind readers about the circular reference?

chia7712 · 2024-06-12T10:00:34Z

@mimaison Could you please take a look at this PR? I noticed this issue in testing dynamic configs, and it seems that was introduced by #16133 :_

frankvicky · 2024-06-12T11:40:40Z

Hi @chia7712, I have do a little changes based on comment, PTAL 🐧

mimaison

Good catch! I just left a suggestion

mimaison · 2024-06-12T12:57:03Z


-  object Broker {
-    private val brokerConfigs = QuotaConfigs.brokerQuotaConfigs()
-    DynamicBrokerConfig.addDynamicConfigs(brokerConfigs)


So DynamicBrokerConfig.addDynamicConfigs() is not used anymore, we can delete it.

Thank you, I will remove it 😄

mimaison · 2024-06-12T13:15:02Z

Thanks for finding and fixing this!
In #16133 I was trying to break this cyclic dependency and also remove the KafkaConfig dependency of one of these classes but they are really badly entangled. I think your change is the best we can do for now.

chia7712 · 2024-06-12T13:49:15Z

In #16133 I was trying to break this cyclic dependency and also remove the KafkaConfig dependency of one of these classes but they are really badly entangled. I think your change is the best we can do for now.In #16133 I was trying to break this cyclic dependency and also remove the KafkaConfig dependency of one of these classes but they are really badly entangled. I think your change is the best we can do for now.

I check the commit history before creating KAFKA-16938, and then ... the reviewer list shows my name 🥲

chia7712 · 2024-06-12T13:53:51Z

discuss with @frankvicky offline. It seems some tests get flaky with this fix. we will go back here later.

BTW, #16133 is in trunk only so this issue won't hurt 3.7.1 and 3.8.0

chia7712 · 2024-06-12T15:07:51Z

we will go back here later.

ok, that is false failure.

chia7712

@frankvicky thanks for this patch. one last comment is left.

chia7712 · 2024-06-12T15:15:46Z

-  object Broker {
-    private val brokerConfigs = QuotaConfigs.brokerQuotaConfigs()
-    DynamicBrokerConfig.addDynamicConfigs(brokerConfigs)
+    // Non-dynamic properties are determined by subtracting dynamic broker config names from all config names.


The key point is DynamicBrokerConfig should NOT have any val variables which are initialized by DynamicConfig.Broker. Hence, the comments can be tweaked to

In order to avoid circular reference, all DynamicBrokerConfig's variables which are initialized by `DynamicConfig.Broker` should be moved to `DynamicConfig.Broker`. Otherwise, those variables of DynamicBrokerConfig will see intermediate state of `DynamicConfig.Broker`, because `brokerConfigs` is created by `DynamicBrokerConfig.AllDynamicConfigs`

I see, I will modify it soon.

chia7712

LGTM

@tag

commit f380cd1 Author: Edoardo Comar <ecomar@uk.ibm.com> Date: Thu Jun 13 15:01:08 2024 +0100 MINOR: Add integration tag to AdminFenceProducersIntegrationTest (apache#16326) Add @tag("integration") to AdminFenceProducersIntegrationTest Reviewers: Chris Egerton <chrise@aiven.io> commit 11c85a9 Author: Dongnuo Lyu <139248811+dongnuo123@users.noreply.github.com> Date: Thu Jun 13 05:11:01 2024 -0400 MINOR: Make online downgrade failure logs less noisy and update the timeouts scheduled in `convertToConsumerGroup` (apache#16290) This patch: - changes the order of the checks in `validateOnlineDowngrade`, so that only when the last member using the consumer protocol leave and the group still has classic member(s), `online downgrade is disabled` is logged if the policy doesn't allow downgrade. - changes the session timeout in `convertToConsumerGroup` from `consumerGroupSessionTimeoutMs` to `member.classicProtocolSessionTimeout().get()`. Reviewers: David Jacot <djacot@confluent.io> commit ea60666 Author: Ken Huang <100591800+m1a2st@users.noreply.github.com> Date: Thu Jun 13 17:11:37 2024 +0900 KAFKA-16921 [1/N] Migrate all junit 4 code to junit 5 for connect module (apache#16253) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit 596b945 Author: gongxuanzhang <gongxuanzhang@foxmail.com> Date: Thu Jun 13 15:39:32 2024 +0800 KAFKA-16643 Add ModifierOrder checkstyle rule (apache#15890) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit 103ff5c Author: Antoine Pourchet <antoine@responsive.dev> Date: Thu Jun 13 01:32:39 2024 -0600 KAFKA-15045: (KIP-924 pt. 24) internal TaskAssignor rename to LegacyTaskAssignor (apache#16318) Since the new public API for TaskAssignor shared a name, this rename will prevent users from confusing the internal definition with the public one. Reviewers: Anna Sophie Blee-Goldman <ableegoldman@apache.org> commit e59c887 Author: brenden20 <118419078+brenden20@users.noreply.github.com> Date: Thu Jun 13 02:30:05 2024 -0500 KAFKA-16557 Implemented OffsetFetchRequestState toStringBase and added a test for it (apache#16291) Reviewers: Lianet Magrans <lianetmr@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com> commit dd6fcc6 Author: TingIāu "Ting" Kì <kitingiao@gmail.com> Date: Thu Jun 13 14:35:33 2024 +0800 KAFKA-16901 Add unit tests for ConsumerRecords#records(String) (apache#16227) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit fe98888 Author: Lianet Magrans <98415067+lianetm@users.noreply.github.com> Date: Thu Jun 13 08:31:16 2024 +0200 MINOR: Improving log for outstanding requests on close and cleanup (apache#16304) Reviewers: Andrew Schofield <aschofield@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com> commit 9ddd58b Author: Chris Egerton <chrise@aiven.io> Date: Thu Jun 13 05:43:33 2024 +0200 MINOR: Add readiness check for connector and separate Kafka cluster in ExactlyOnceSourceIntegrationTest::testSeparateOffsetsTopic (apache#16306) Reviewers: Greg Harris <gharris1727@gmail.com> commit 0a203a9 Author: TingIāu "Ting" Kì <kitingiao@gmail.com> Date: Thu Jun 13 09:47:51 2024 +0800 KAFKA-16938 non-dynamic props gets corrupted due to circular reference between DynamicBrokerConfig and DynamicConfig. (apache#16302) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit 6d1f8f8 Author: Gantigmaa Selenge <39860586+tinaselenge@users.noreply.github.com> Date: Thu Jun 13 02:42:39 2024 +0100 MINOR: Clean up for KafkaAdminClientTest (apache#16285) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit e76e1da Author: Chris Egerton <chrise@aiven.io> Date: Thu Jun 13 02:18:23 2024 +0200 KAFKA-16935: Automatically wait for cluster startup in embedded Connect integration tests (apache#16288) Reviewers: Greg Harris <gharris1727@gmail.com>

@tag

commit 4333af5 Author: A. Sophie Blee-Goldman <ableegoldman@gmail.com> Date: Thu Jun 13 11:27:50 2024 -0700 KAFKA-15045: (KIP-924 pt. 25) Rename old internal StickyTaskAssignor to LegacyStickyTaskAssignor (apache#16322) To avoid confusion in 3.8/until we fully remove all the old task assignors and internal config, we should rename the old internal assignor classes like the StickyTaskAssignor so that they won't be mixed up with the new version of the assignor (which is also named StickyTaskAssignor) Reviewers: Bruno Cadonna <cadonna@apache.org>, Josep Prat <josep.prat@aiven.io> commit f380cd1 Author: Edoardo Comar <ecomar@uk.ibm.com> Date: Thu Jun 13 15:01:08 2024 +0100 MINOR: Add integration tag to AdminFenceProducersIntegrationTest (apache#16326) Add @tag("integration") to AdminFenceProducersIntegrationTest Reviewers: Chris Egerton <chrise@aiven.io> commit 11c85a9 Author: Dongnuo Lyu <139248811+dongnuo123@users.noreply.github.com> Date: Thu Jun 13 05:11:01 2024 -0400 MINOR: Make online downgrade failure logs less noisy and update the timeouts scheduled in `convertToConsumerGroup` (apache#16290) This patch: - changes the order of the checks in `validateOnlineDowngrade`, so that only when the last member using the consumer protocol leave and the group still has classic member(s), `online downgrade is disabled` is logged if the policy doesn't allow downgrade. - changes the session timeout in `convertToConsumerGroup` from `consumerGroupSessionTimeoutMs` to `member.classicProtocolSessionTimeout().get()`. Reviewers: David Jacot <djacot@confluent.io> commit ea60666 Author: Ken Huang <100591800+m1a2st@users.noreply.github.com> Date: Thu Jun 13 17:11:37 2024 +0900 KAFKA-16921 [1/N] Migrate all junit 4 code to junit 5 for connect module (apache#16253) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit 596b945 Author: gongxuanzhang <gongxuanzhang@foxmail.com> Date: Thu Jun 13 15:39:32 2024 +0800 KAFKA-16643 Add ModifierOrder checkstyle rule (apache#15890) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit 103ff5c Author: Antoine Pourchet <antoine@responsive.dev> Date: Thu Jun 13 01:32:39 2024 -0600 KAFKA-15045: (KIP-924 pt. 24) internal TaskAssignor rename to LegacyTaskAssignor (apache#16318) Since the new public API for TaskAssignor shared a name, this rename will prevent users from confusing the internal definition with the public one. Reviewers: Anna Sophie Blee-Goldman <ableegoldman@apache.org> commit e59c887 Author: brenden20 <118419078+brenden20@users.noreply.github.com> Date: Thu Jun 13 02:30:05 2024 -0500 KAFKA-16557 Implemented OffsetFetchRequestState toStringBase and added a test for it (apache#16291) Reviewers: Lianet Magrans <lianetmr@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com> commit dd6fcc6 Author: TingIāu "Ting" Kì <kitingiao@gmail.com> Date: Thu Jun 13 14:35:33 2024 +0800 KAFKA-16901 Add unit tests for ConsumerRecords#records(String) (apache#16227) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit fe98888 Author: Lianet Magrans <98415067+lianetm@users.noreply.github.com> Date: Thu Jun 13 08:31:16 2024 +0200 MINOR: Improving log for outstanding requests on close and cleanup (apache#16304) Reviewers: Andrew Schofield <aschofield@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com> commit 9ddd58b Author: Chris Egerton <chrise@aiven.io> Date: Thu Jun 13 05:43:33 2024 +0200 MINOR: Add readiness check for connector and separate Kafka cluster in ExactlyOnceSourceIntegrationTest::testSeparateOffsetsTopic (apache#16306) Reviewers: Greg Harris <gharris1727@gmail.com> commit 0a203a9 Author: TingIāu "Ting" Kì <kitingiao@gmail.com> Date: Thu Jun 13 09:47:51 2024 +0800 KAFKA-16938 non-dynamic props gets corrupted due to circular reference between DynamicBrokerConfig and DynamicConfig. (apache#16302) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit 6d1f8f8 Author: Gantigmaa Selenge <39860586+tinaselenge@users.noreply.github.com> Date: Thu Jun 13 02:42:39 2024 +0100 MINOR: Clean up for KafkaAdminClientTest (apache#16285) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit e76e1da Author: Chris Egerton <chrise@aiven.io> Date: Thu Jun 13 02:18:23 2024 +0200 KAFKA-16935: Automatically wait for cluster startup in embedded Connect integration tests (apache#16288) Reviewers: Greg Harris <gharris1727@gmail.com>

KAFKA-16938: Break circular reference between DynamicBrokerConfig and…

0658846

… DynamicConfig

chia7712 reviewed Jun 12, 2024

View reviewed changes

KAFKA-16938: Add comment and correct indentation

88ac773

mimaison reviewed Jun 12, 2024

View reviewed changes

KAFKA-16938: Remove unused method addDynamicConfigs

a59fa3f

chia7712 reviewed Jun 12, 2024

View reviewed changes

frankvicky added 2 commits June 12, 2024 23:20

KAFKA-16938: Rewrite comment to make it more precise

e27c225

KAFKA-16938: Format comment

40e992c

chia7712 approved these changes Jun 12, 2024

View reviewed changes

chia7712 merged commit 0a203a9 into apache:trunk Jun 13, 2024

Conversation

frankvicky commented Jun 12, 2024

Committer Checklist (excluded from commit message)

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

chia7712 Jun 12, 2024

Choose a reason for hiding this comment

Uh oh!

frankvicky Jun 12, 2024

Choose a reason for hiding this comment

Uh oh!

chia7712 Jun 12, 2024

Choose a reason for hiding this comment

Uh oh!

chia7712 commented Jun 12, 2024

Uh oh!

frankvicky commented Jun 12, 2024

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

mimaison Jun 12, 2024

Choose a reason for hiding this comment

Uh oh!

frankvicky Jun 12, 2024

Choose a reason for hiding this comment

Uh oh!

mimaison commented Jun 12, 2024

Uh oh!

chia7712 commented Jun 12, 2024

Uh oh!

chia7712 commented Jun 12, 2024

Uh oh!

chia7712 commented Jun 12, 2024

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

chia7712 Jun 12, 2024

Choose a reason for hiding this comment

Uh oh!

frankvicky Jun 12, 2024

Choose a reason for hiding this comment

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants