KAFKA-16976: Update the current/dynamic config inside RemoteLogManagerConfig by kamalcph · Pull Request #16394 · apache/kafka

kamalcph · 2024-06-19T06:46:23Z

Reverted the some of the changes that was done in #16353

Committer Checklist (excluded from commit message)

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

…fig.

kamalcph · 2024-06-19T06:47:24Z

@chiacyu

Call for review. PTAL.

chia7712 · 2024-06-19T14:05:09Z

+     * @param props
+     */
+    public void updateCurrentConfig(Map<?, ?> props) {
+        if (props.containsKey(REMOTE_LOG_INDEX_FILE_CACHE_TOTAL_SIZE_BYTES_PROP)) {


It seems to me this way has following disadvantages.

it is not thread-safe

those configs must be equal to DynamicRemoteLogConfig#ReconfigurableConfigs, and it could be easy to forget to make them consistency

there are two sources of those configs - props and inner variable - that is a bit chaos

If we want to make all "created" RemoteLogManagerConfig can see "latest" configs and fix above disadvantages, a simple solution is RemoteLogManagerConfig can keep a reference of KafkaConfig instead of having KafkaConfig snapshot.

WDYT?

Agreed with @chia7712 ,
we should keep the consistency along with thread-safty.

We cannot add the 'core' module dependency to the 'storage' module. So, kafka.core.KafkaConfig can't be referenced in RemoteLogManagerConfig.

there are two sources of those configs - props and inner variable - that is a bit chaos

One option is to follow the approach that you have suggested in this comment to avoid exposing the unrelated/unexpected methods.

We cannot add the 'core' module dependency to the 'storage' module. So, kafka.core.KafkaConfig can't be referenced in RemoteLogManagerConfig.

My point was to use KafkaConfig to create RemoteLogManagerConfig, and so RemoteLogManagerConfig will have a reference to AbstractConfig ( and its point is KafkaConfig). For example:

public RemoteLogManagerConfig(AbstractConfig configs)

Also, my another concern is - how we handle other similar configs in the future? add all of them to updateCurrentConfig (it means they have two kinds of configs sources - props and individual dynamic volatile fields) ? or make them reuse a class which can reflect the latest configs?

Small suggestions (I am thinking out loud here) KafkaConfig in server is now inheriting AbstractKafkaConfig and am already working on moving most getters into AbstractKafkaConfig here #16307 so maybe we can refer to AbstractKafkaConfig instead of AbstractConfig and any getters you need define it in AbstractKafkaConfig. Specially that AbstractKafkaConfig is going to replace KafkaConfig at some point.

@OmniaGM that will go back to face the issue - AbstractKafkaConfig is in server module :(

any getters you need define it in AbstractKafkaConfig

that is another design we need to discuss. If we will have different config class (for example:RemoteLogManagerConfig) for each component, do we need to add all getters to KafkaConfig/AbstractKafkaConfig? Or we can follow the pattern of RemoteLogManagerConfig that KafkaConfig can create every config class to get related config? That is a trade-off, and currently I prefer the later as it avoids a huge KafkaConfig and KafkaConfig is still a center that we can get all configs from it.

@OmniaGM that will go back to face the issue - AbstractKafkaConfig is in server module :(

:/ yeah this is getting more complicated. I just feel using a very generic AbstractConfig instead of KafkaConfig make the code bit unreadable as you need to really to follow what config is passed.

any getters you need define it in AbstractKafkaConfig

We don't need to keep the getters there for ever however until we move everything out we need a place for KafkaConfig that not core as this is the hard no module to depend on in all the other modules.

:/ yeah this is getting more complicated. I just feel using a very generic AbstractConfig instead of KafkaConfig make the code bit unreadable as you need to really to follow what config is passed.

agree as we leverage KafkaConfig to see "latest" configs, and that is something behind the AbstractConfig.

We don't need to keep the getters there for ever however until we move everything out we need a place for KafkaConfig that not core as this is the hard no module to depend on in all the other modules.

it seems eventually we need a module to run server with all required code, and so I assume server module is the one (but it will get smaller than core module).

Also, AbstractKafkaConfig does a great job that it does not declare all ConfigDef by itself. Instead, it collect all ConfigDef from each config class. That is why I feel we can follow the pattern to deal with other config classes.

chia7712

@kamalcph thanks for adopting the approach. This will be the good reference for other similar issues (#16388, and https://issues.apache.org/jira/browse/KAFKA-16909).

chia7712 · 2024-06-20T13:16:32Z

I'm going to merge this PR. It seems to me the style brought by this PR includes two benefits

return config by AbstractConfig#get - that makes sure all returned configs are up-to-date
move config getters (for instance: remoteLogIndexFileCacheTotalSizeBytes) from KafkaConfig to RemoteLogManagerConfig - that makes KafkaConfig tidier

A comment raised by @OmniaGM (#16394 (comment)) is not addressed and I open https://issues.apache.org/jira/browse/KAFKA-17001 for it

…Config (apache#16394) Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

KAFKA-16976: Update the current/dynamic config in RemoteLogManagerCon…

08e36af

…fig.

kamalcph requested review from chia7712, satishd and showuon June 19, 2024 06:46

kamalcph added the tiered-storage Related to the Tiered Storage feature label Jun 19, 2024

Add unit test

08912b9

chia7712 reviewed Jun 19, 2024

View reviewed changes

chia7712 mentioned this pull request Jun 19, 2024

KAFKA-16908: Refactor QuorumConfig with AbstractConfig #16388

Closed

3 tasks

kamalcph added 3 commits June 19, 2024 22:36

make dynamic config to be thread-safe

2d46ab3

Addressed the review comments.

39b8d92

remove comment

daf36fd

chia7712 approved these changes Jun 20, 2024

View reviewed changes

chia7712 merged commit 4fe08f3 into apache:trunk Jun 20, 2024

kamalcph deleted the KAFKA-16976 branch June 21, 2024 03:24

chia7712 mentioned this pull request Jul 2, 2024

KAFKA-16909 Refactor GroupCoordinatorConfig with AbstractConfig #16458

Merged

3 tasks

TaiJuWu pushed a commit to TaiJuWu/kafka that referenced this pull request Jul 4, 2024

KAFKA-16976 Update the current/dynamic config inside RemoteLogManager…

fc2d30c

…Config (apache#16394) Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

Conversation

kamalcph commented Jun 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

kamalcph commented Jun 19, 2024

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!

OmniaGM Jun 20, 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!

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!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

chia7712 commented Jun 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kamalcph commented Jun 19, 2024 •

edited

Loading

OmniaGM Jun 20, 2024 •

edited

Loading