KAFKA-15853: Move TransactionLogConfig and TransactionStateManagerConfig getters out of KafkaConfig by OmniaGM · Pull Request #16665 · apache/kafka

OmniaGM · 2024-07-23T14:48:14Z

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

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

mimaison · 2024-07-26T15:18:02Z

It looks like this breaks a bunch of tests:

java.lang.NullPointerException
	at kafka.server.KafkaConfig.get(KafkaConfig.scala:212)
	at org.apache.kafka.common.config.AbstractConfig.getInt(AbstractConfig.java:177)

…rConfig

chia7712

@OmniaGM thanks for this nice refactor. some trivial comments are left. Also, please take a look at failed tests.

chia7712 · 2024-07-30T19:51:21Z

-            .defineInternal(TransactionLogConfigs.PRODUCER_ID_EXPIRATION_CHECK_INTERVAL_MS_CONFIG, INT, TransactionLogConfigs.PRODUCER_ID_EXPIRATION_CHECK_INTERVAL_MS_DEFAULT, atLeast(1), LOW, TransactionLogConfigs.PRODUCER_ID_EXPIRATION_CHECK_INTERVAL_MS_DOC);
+            .defineInternal(PRODUCER_ID_EXPIRATION_CHECK_INTERVAL_MS_CONFIG, INT, PRODUCER_ID_EXPIRATION_CHECK_INTERVAL_MS_DEFAULT, atLeast(1), LOW, PRODUCER_ID_EXPIRATION_CHECK_INTERVAL_MS_DOC);
+
+    private AbstractConfig config;


please add final

chia7712 · 2024-07-30T19:51:56Z

+    }
+
+    public Boolean transactionPartitionVerificationEnable() {
+        return config.getBoolean(TRANSACTION_PARTITION_VERIFICATION_ENABLE_CONFIG);


could you please add comments to for those dynamic configs?

Even with the comments, I'm not sure I understand why this is not initialized in the constructor.

Can you explain how this is supposed to work? AbstractConfig instances are kind of read only by default but it looks like we're expecting it to change here. Am I missing something?

we had a discussion for that before (#16458 (comment))

AbstractConfig is read-only, but the sub class KafkaConfig can update inner configs dynamically (see https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/server/KafkaConfig.scala#L192). Hence, the passed AbstractConfig always have "up-to-date configs"

There is a jira which tries to refactor it (https://issues.apache.org/jira/browse/KAFKA-17001), but I haven't worked on it because that will introduce major changes to scala code.

In short, the basic rules for now are shown below.

the non-dynamic config will be evaluated in constructor.

the dynamic config will be evaluated in getter (as AbstractConfig always have latest configs)

Thanks for the explanations!

chia7712 · 2024-07-30T19:53:16Z

+            .define(TRANSACTIONS_ABORT_TIMED_OUT_TRANSACTION_CLEANUP_INTERVAL_MS_CONFIG, INT, TRANSACTIONS_ABORT_TIMED_OUT_TRANSACTION_CLEANUP_INTERVAL_MS_DEFAULT, atLeast(1), LOW, TRANSACTIONS_ABORT_TIMED_OUT_TRANSACTIONS_INTERVAL_MS_DOC)
+            .define(TRANSACTIONS_REMOVE_EXPIRED_TRANSACTIONAL_ID_CLEANUP_INTERVAL_MS_CONFIG, INT, TRANSACTIONS_REMOVE_EXPIRED_TRANSACTIONAL_ID_CLEANUP_INTERVAL_MS_DEFAULT, atLeast(1), LOW, TRANSACTIONS_REMOVE_EXPIRED_TRANSACTIONS_INTERVAL_MS_DOC);
+
+    private int transactionalIdExpirationMs;


please add final

chia7712 · 2024-08-01T17:26:38Z

@OmniaGM I love this PR, but it includes too many files. Hence, I'd like to delay this PR until 3.9.0 gets release :)

mimaison · 2024-08-08T16:19:17Z

So is this the format we're going for? I see we have a constructor accepting AbstractConfig and getters for all fields, plus potentially a validate() method for any validations that only rely on configs in the class.

I think it would be good to document the expected target and explain the pros and cons vs alternatives in the Jira.

chia7712 · 2024-08-18T19:49:00Z

@OmniaGM could you please fix the conflicts? thanks!

chia7712 · 2024-08-19T21:28:25Z

@OmniaGM Both TransactionStateManagerConfig and TransactionLogConfig are small file, and they are normally called together. Hence, do you consider merging them into one file?

OmniaGM · 2024-08-20T09:48:46Z

@OmniaGM Both TransactionStateManagerConfig and TransactionLogConfig are small file, and they are normally called together. Hence, do you consider merging them into one file?

They are called together only KafkaConfig, TransactionStateManager, TransactionCoordinator, LogManager. While TransactionLogConfig is used in DynamicBrokerConfig, KafkaServer, AutoTopicCreationManager, ReplicaManager, BrokerMetadataPublisher. I split them into 2 previously because the comment on them in KafkaConfig back then suggested they are different domains. I guess we can move TransactionLogConfig into TransactionStateManagerConfig. I just feel like the scope of where each one is used is bit different but generally the code access them from KafkaConfig anyway.

chia7712 · 2024-08-20T10:55:53Z

I guess we can move TransactionLogConfig into TransactionStateManagerConfig. I just feel like the scope of where each one is used is bit different but generally the code access them from KafkaConfig anyway.

I have no strong reason to merge them though 😃

Let's keep current style. I will take another look later

chia7712 · 2024-09-01T15:47:31Z

@OmniaGM Sorry that could you please fix the conflicts again? This PR will have highest priority in my queue

…ig getters out of KafkaConfig (apache#16665) Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

OmniaGM changed the title ~~KAFKA-15853: Move TransactionLogConfig and TransactionStateManagerConfig getters out of K~~ KAFKA-15853: Move TransactionLogConfig and TransactionStateManagerConfig getters out of KafkaConfig Jul 23, 2024

KAFKA-15853: Refactor TransactionLogConfig and TransactionStateManage…

d643e8c

…rConfig

OmniaGM force-pushed the KAFKA-15853-txnConfig branch from 6959e4b to d643e8c Compare July 29, 2024 10:56

Merge remote-tracking branch 'apache' into KAFKA-15853-txnConfig

fef8123

chia7712 reviewed Jul 30, 2024

View reviewed changes

OmniaGM force-pushed the KAFKA-15853-txnConfig branch from 74a880a to 76d33d3 Compare August 1, 2024 14:05

address @chia7712's feedback

5a4d3d5

OmniaGM force-pushed the KAFKA-15853-txnConfig branch from 76d33d3 to 5a4d3d5 Compare August 1, 2024 14:07

OmniaGM added 2 commits August 13, 2024 14:21

Merge remote-tracking branch 'apache/trunk' into KAFKA-15853-txnConfig

2c57877

Merge remote-tracking branch 'apache/trunk' into KAFKA-15853-txnConfig

801dca0

Merge remote-tracking branch 'apache/trunk' into KAFKA-15853-txnConfig

c754ca1

Merge remote-tracking branch 'apache/trunk' into KAFKA-15853-txnConfig

fbedd41

Merge remote-tracking branch 'apache/trunk' into KAFKA-15853-txnConfig

1396b7d

chia7712 approved these changes Sep 3, 2024

View reviewed changes

chia7712 merged commit f59d829 into apache:trunk Sep 3, 2024

bboyleonp666 pushed a commit to bboyleonp666/kafka that referenced this pull request Sep 4, 2024

KAFKA-15853 Move TransactionLogConfig and TransactionStateManagerConf…

b2aa8b9

…ig getters out of KafkaConfig (apache#16665) Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

tedyu pushed a commit to tedyu/kafka that referenced this pull request Jan 6, 2025

KAFKA-15853 Move TransactionLogConfig and TransactionStateManagerConf…

83305c5

…ig getters out of KafkaConfig (apache#16665) Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

Conversation

OmniaGM commented Jul 23, 2024

Committer Checklist (excluded from commit message)

Uh oh!

mimaison commented Jul 26, 2024

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

chia7712 Jul 30, 2024

Choose a reason for hiding this comment

Uh oh!

chia7712 Jul 30, 2024

Choose a reason for hiding this comment

Uh oh!

mimaison Aug 8, 2024

Choose a reason for hiding this comment

Uh oh!

chia7712 Aug 8, 2024

Choose a reason for hiding this comment

Uh oh!

mimaison Aug 12, 2024

Choose a reason for hiding this comment

Uh oh!

chia7712 Jul 30, 2024

Choose a reason for hiding this comment

Uh oh!

chia7712 commented Aug 1, 2024

Uh oh!

mimaison commented Aug 8, 2024

Uh oh!

chia7712 commented Aug 18, 2024

Uh oh!

chia7712 commented Aug 19, 2024

Uh oh!

OmniaGM commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chia7712 commented Aug 20, 2024

Uh oh!

chia7712 commented Sep 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

OmniaGM commented Aug 20, 2024 •

edited

Loading