KAFKA-16884 : Refactor RemoteLogManagerConfig with AbstractConfig #16199

muralibasani · 2024-06-04T19:32:08Z

Resolves https://issues.apache.org/jira/browse/KAFKA-16884

Update RemoteLogManagerConfig, extend with AbstractConfig
Update all the dependent getValue methods

Committer Checklist (excluded from commit message)

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

muralibasani · 2024-06-04T19:37:52Z

@gharris1727 would you like to take a look ?

gharris1727

Hey thanks @muralibasani for the PR! I had a comment that can make the PR a lot smaller :)

core/src/main/java/kafka/log/remote/RemoteLogManager.java

...age/src/test/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfigTest.java

gharris1727 · 2024-06-04T21:12:48Z

storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfig.java

-                            remoteStorageManagerPrefix, remoteLogMetadataManagerPrefix, remoteLogManagerCopyMaxBytesPerSecond,
-                            remoteLogManagerCopyNumQuotaSamples, remoteLogManagerCopyQuotaWindowSizeSeconds, remoteLogManagerFetchMaxBytesPerSecond,
-                            remoteLogManagerFetchNumQuotaSamples, remoteLogManagerFetchQuotaWindowSizeSeconds);
+    protected RemoteLogManagerConfig(Map<?, ?> props, boolean doLog) {


This constructor isn't doing anything. You can call super(ConfigDef, Map) in the other constructor.

Also don't forget to remove CONFIG_DEF and replace it with a ConfigDef configDef() static method.

It seems to me composition is better than inheritance. Extending AbstractConfig may add many unrelated/unexpected methods to RemoteLogManagerConfig. Being a POJO is more safe.

Hence, maybe we can keep a inner AbstractConfig object? All getters can be re-implemented by that inner AbstractConfig

I understand the maxim of composition over inheritance, but personally I value codebase consistency more.
Do you have a more specific justification in this case that would motivate us to migrate all other AbstractConfig subclasses to composition?

After all, the AbstractConfig documentation promotes this style:

kafka/clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java

Line 41 in f2aafcc

* A convenient base class for configurations to extend.

I value codebase consistency more.
After all, the AbstractConfig documentation promotes this style:

ya, that is a good reason. However, the rule is written in 2014 and I don't think "to extend" means "can't compose"

Do you have a more specific justification in this case that would motivate us to migrate all other AbstractConfig subclasses to composition?

that is not what I try to say. We don't need to change whole code base when we are aware of good pattern, but it would be nice to apply on the new code.

the design pattern is a long story. I love to have more discussion, but it is not worth obstructing the progress from this PR.

Please ignore my comments and let's move on this PR :)

However, the rule is written in 2014 and I don't think "to extend" means "can't compose"

I completely agree. There are lots of decisions from 2014/2015 that I disagree with and actively cause me problems, this one fortunately hasn't been one of them.

We don't need to change whole code base when we are aware of good pattern, but it would be nice to apply on the new code.

I agree that the two styles can coexist at the same time, and implementing a pattern on "new" code is a good rollout plan. I do think that one should eventually "win", and I was curious if you had a good reason that might make composition win :)

I was curious if you had a good reason that might make composition win :)

There are two main reasons.

Extending the AbstractConfig will have a bunch of "public" APIs. Those unused public APIs will make subclass verbose in using

the exposed APIs may mislead callers to take "unrelated" configs from RemoteLogManagerConfig. And that may cause more consistency in the future

...age/src/test/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfigTest.java

kamalcph

Thanks for the patch! Should we also look into cases where the configs gets updated dynamically?

KafkaConfig handles the dynamic configuration update using the currentConfig. Should we also handle it in RemoteLogManagerConfig?

storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfig.java

gharris1727 · 2024-06-05T15:58:24Z

storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfig.java

+    public static final ConfigDef CONFIG_DEF;
+
+    public static ConfigDef configDef() {
+        return CONFIG_DEF;


This is still a mutable shared object. configDef() should execute all of the code in the static initializer each time it is called to produce a new ConfigDef.

@muralibasani Please see

kafka/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/ConnectorConfig.java

Line 201 in 8b3c77c

public static ConfigDef configDef() {

CONFIG_DEF is still mutable and shouldn't exist.

Got it, updated it similar to ConnectorConfig

With this change, had to make another change in KafkaConfig.scala https://github.com/apache/kafka/pull/16199/files#diff-cbe6a8b71b05ed22cf09d97591225b588e9fca6caaf95d3b34a43262cfd23aa6R458

@gharris1727 However in AdminClientConfig and a few other configs, I notice config objects are mutable

It's a pretty common mistake it seems. It is more of a risk when working with arbitrary plugins, but is still good practice everywhere.

It would have been nice to have ConfigDef be immutable, and have the define() methods on a ConfigDefBuilder instead. But the design we have now is very entrenched, so we can just cope with it.

...age/src/test/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfigTest.java

gharris1727

LGTM. I'll wait for another pass from @chia7712 and/or @kamalcph. Thanks!

chia7712

@muralibasani thanks for this patch. overall LGTM. two small comments are left.

chia7712 · 2024-06-05T19:22:40Z

storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfig.java

    }

    public Map<String, Object> remoteLogMetadataManagerProps() {
-        return Collections.unmodifiableMap(remoteLogMetadataManagerProps);
+        return Collections.unmodifiableMap(getString(REMOTE_LOG_METADATA_MANAGER_CONFIG_PREFIX_PROP) != null


Wrapping a empty map by Collections.unmodifiableMap is a bit awkward. Maybe we can check the existence first? For example:

return remoteLogMetadataManagerPrefix() != null ? Collections.unmodifiableMap(originalsWithPrefix(remoteLogMetadataManagerPrefix())) : Collections.emptyMap();

@chia7712 From the previous code, probably it should be unmodifiable even if it's empty ? Hence I kept the same without breaking any existing code

emptyMap() is itself immutable.

emptyMap() is itself immutable.

yep, thanks for quick response :)

Nice one. Updated.

chia7712 · 2024-06-05T19:22:45Z

storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfig.java

    }

    public Map<String, Object> remoteStorageManagerProps() {
-        return Collections.unmodifiableMap(remoteStorageManagerProps);
+        return Collections.unmodifiableMap(getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP) != null


chia7712 · 2024-06-05T20:40:42Z

storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfig.java

+        Map<String, Object> configProps = (prefixProp != null)
+                ? originalsWithPrefix(prefixProp)
+                : Collections.emptyMap();
+        return configProps.isEmpty() ? configProps : Collections.unmodifiableMap(configProps);


What if originalsWithPrefix(prefixProp) return empty map? IIRC, RecordingMap is modifiable. Maybe return prefixProp == null ? Collections.emptyMap() : Collections.unmodifiableMap(originalsWithPrefix(prefixProp)); is enough good.

Agree, after inverting the condition, this looks really good. Updated.

chia7712

LGTM

kamalcph

Overall LGTM, left few minor comments.

kamalcph · 2024-06-06T04:00:58Z

...age/src/test/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfigTest.java

+        // Even with empty properties, RemoteLogManagerConfig has default values
+        Map<String, Object> emptyProps = new HashMap<>();
+        RemoteLogManagerConfig remoteLogManagerConfigEmptyConfig = new RemoteLogManagerConfig(emptyProps);
+        assertEquals(remoteLogManagerConfigEmptyConfig.remoteLogManagerThreadPoolSize(), DEFAULT_REMOTE_LOG_MANAGER_THREAD_POOL_SIZE);


nit: swap the arguments

assertEquals(expected, actual)

kamalcph · 2024-06-06T04:01:07Z

...age/src/test/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfigTest.java

+        Map<String, Object> emptyProps = new HashMap<>();
+        RemoteLogManagerConfig remoteLogManagerConfigEmptyConfig = new RemoteLogManagerConfig(emptyProps);
+        assertEquals(remoteLogManagerConfigEmptyConfig.remoteLogManagerThreadPoolSize(), DEFAULT_REMOTE_LOG_MANAGER_THREAD_POOL_SIZE);
+        assertEquals(remoteLogManagerConfigEmptyConfig.remoteLogManagerCopyNumQuotaSamples(), DEFAULT_REMOTE_LOG_MANAGER_COPY_QUOTA_WINDOW_NUM);


nit: swap the arguments

assertEquals(expected, actual)

kamalcph · 2024-06-06T04:02:49Z

...age/src/test/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfigTest.java

        Map<String, Object> props = new HashMap<>();
-        props.put(RemoteLogManagerConfig.REMOTE_LOG_STORAGE_SYSTEM_ENABLE_PROP,
-                  remoteLogManagerConfig.enableRemoteStorageSystem());
+        props.put(REMOTE_LOG_STORAGE_SYSTEM_ENABLE_PROP, true);


nit:

some configs are with prefix and some are referenced using static import. Please stick to either one format.

@kamalcph yea, fixed now. thanks.

kamalcph · 2024-06-06T04:15:15Z

We want to update the below configs dynamically:

remote.log.manager.copier.thread.pool.size
remote.log.manager.expiration.thread.pool.size
remote.log.manager.thread.pool.size and
remote.fetch.max.wait.ms

Dynamic values gets reflected in two ways:

A scheduler which checks the value (kafkaConfig.remoteLogManagerConfig.xyz) and performs an action. Once the config gets updated dynamically, it takes effect in the next run.
Continuous running job, once the config is updated we have to invoke those respective components to take effect the updated config change.

How to support dynamic configs for case-1 via this class? KafkaConfig has a nice abstraction of currentConfig which handles the dynamic config. One way to do is to move the getter methods from RLMConfig to KafkaConfig to reflect the updated values.

Would like to get your thoughts on this. We can take this up separately.

kamalcph

LGTM

muralibasani · 2024-06-06T09:07:51Z

@kamalcph @gharris1727 @chia7712 thanks for all your great reviews.

chia7712 · 2024-06-06T16:13:42Z

@muralibasani thanks for you to give this nice refactor

commit ee834d9 Author: Antoine Pourchet <[email protected]> Date: Thu Jun 6 15:20:48 2024 -0600 KAFKA-15045: (KIP-924 pt. 19) Update to new AssignmentConfigs (apache#16219) This PR updates all of the streams task assignment code to use the new AssignmentConfigs public class. Reviewers: Anna Sophie Blee-Goldman <[email protected]> commit 8a2bc3a Author: Bruno Cadonna <[email protected]> Date: Thu Jun 6 21:19:52 2024 +0200 KAFKA-16903: Consider produce error of different task (apache#16222) A task does not know anything about a produce error thrown by a different task. That might lead to a InvalidTxnStateException when a task attempts to do a transactional operation on a producer that failed due to a different task. This commit stores the produce exception in the streams producer on completion of a send instead of the record collector since the record collector is on task level whereas the stream producer is on stream thread level. Since all tasks use the same streams producer the error should be correctly propagated across tasks of the same stream thread. For EOS alpha, this commit does not change anything because each task uses its own producer. The send error is still on task level but so is also the transaction. Reviewers: Matthias J. Sax <[email protected]> commit 7d832cf Author: David Jacot <[email protected]> Date: Thu Jun 6 21:19:20 2024 +0200 KAFKA-14701; Move `PartitionAssignor` to new `group-coordinator-api` module (apache#16198) This patch moves the `PartitionAssignor` interface and all the related classes to a newly created `group-coordinator/api` module, following the pattern used by the storage and tools modules. Reviewers: Ritika Reddy <[email protected]>, Jeff Kim <[email protected]>, Chia-Ping Tsai <[email protected]> commit 79ea7d6 Author: Mickael Maison <[email protected]> Date: Thu Jun 6 20:28:39 2024 +0200 MINOR: Various cleanups in clients (apache#16193) Reviewers: Chia-Ping Tsai <[email protected]> commit a41f7a4 Author: Murali Basani <[email protected]> Date: Thu Jun 6 18:06:25 2024 +0200 KAFKA-16884 Refactor RemoteLogManagerConfig with AbstractConfig (apache#16199) Reviewers: Greg Harris <[email protected]>, Kamal Chandraprakash <[email protected]>, Chia-Ping Tsai <[email protected]> commit 0ed104c Author: Kamal Chandraprakash <[email protected]> Date: Thu Jun 6 21:26:08 2024 +0530 MINOR: Cleanup the storage module unit tests (apache#16202) - Use SystemTime instead of MockTime when time is not mocked - Use static assertions to reduce the line length - Fold the lines if it exceeds the limit - rename tp0 to tpId0 when it refers to TopicIdPartition Reviewers: Kuan-Po (Cooper) Tseng <[email protected]>, Chia-Ping Tsai <[email protected]> commit f36a873 Author: Cy <[email protected]> Date: Thu Jun 6 08:46:49 2024 -0700 MINOR: Added test for ClusterConfig#displayTags (apache#16110) Reviewers: Chia-Ping Tsai <[email protected]> commit 226f3c5 Author: Sanskar Jhajharia <[email protected]> Date: Thu Jun 6 18:48:23 2024 +0530 MINOR: Code cleanup in metadata module (apache#16065) Reviewers: Mickael Maison <[email protected]> commit ebe1e96 Author: Loïc GREFFIER <[email protected]> Date: Thu Jun 6 13:40:31 2024 +0200 KAFKA-16448: Add ProcessingExceptionHandler interface and implementations (apache#16187) This PR is part of KAFKA-16448 which aims to bring a ProcessingExceptionHandler to Kafka Streams in order to deal with exceptions that occur during processing. This PR brings ProcessingExceptionHandler interface and default implementations. Co-authored-by: Dabz <[email protected]> Co-authored-by: sebastienviale <[email protected]> Reviewer: Bruno Cadonna <[email protected]> commit b74b182 Author: Lianet Magrans <[email protected]> Date: Thu Jun 6 09:45:36 2024 +0200 KAFKA-16786: Remove old assignment strategy usage in new consumer (apache#16214) Remove usage of the partition.assignment.strategy config in the new consumer. This config is deprecated with the new consumer protocol, so the AsyncKafkaConsumer should not use or validate the property. Reviewers: Lucas Brutschy <[email protected]> commit f880ad6 Author: Alyssa Huang <[email protected]> Date: Wed Jun 5 23:30:49 2024 -0700 KAFKA-16530: Fix high-watermark calculation to not assume the leader is in the voter set (apache#16079) 1. Changing log message from error to info - We may expect the HW calculation to give us a smaller result than the current HW in the case of quorum reconfiguration. We will continue to not allow the HW to actually decrease. 2. Logic for finding the updated LeaderEndOffset for updateReplicaState is changed as well. We do not assume the leader is in the voter set and check the observer states as well. 3. updateLocalState now accepts an additional "lastVoterSet" param which allows us to update the leader state with the last known voters. any nodes in this set but not in voterStates will be added to voterStates and removed from observerStates, any nodes not in this set but in voterStates will be removed from voterStates and added to observerStates Reviewers: Luke Chen <[email protected]>, José Armando García Sancio <[email protected]> commit 3835515 Author: Okada Haruki <[email protected]> Date: Thu Jun 6 15:10:13 2024 +0900 KAFKA-16541 Fix potential leader-epoch checkpoint file corruption (apache#15993) A patch for KAFKA-15046 got rid of fsync on LeaderEpochFileCache#truncateFromStart/End for performance reason, but it turned out this could cause corrupted leader-epoch checkpoint file on ungraceful OS shutdown, i.e. OS shuts down in the middle when kernel is writing dirty pages back to the device. To address this problem, this PR makes below changes: (1) Revert LeaderEpochCheckpoint#write to always fsync (2) truncateFromStart/End now call LeaderEpochCheckpoint#write asynchronously on scheduler thread (3) UnifiedLog#maybeCreateLeaderEpochCache now loads epoch entries from checkpoint file only when current cache is absent Reviewers: Jun Rao <[email protected]> commit 7763243 Author: Florin Akermann <[email protected]> Date: Thu Jun 6 00:22:31 2024 +0200 KAFKA-12317: Update FK-left-join documentation (apache#15689) FK left-join was changed via KIP-962. This PR updates the docs accordingly. Reviewers: Ayoub Omari <[email protected]>, Matthias J. Sax <[email protected]> commit 1134520 Author: Ayoub Omari <[email protected]> Date: Thu Jun 6 00:05:04 2024 +0200 KAFKA-16573: Specify node and store where serdes are needed (apache#15790) Reviewers: Matthias J. Sax <[email protected]>, Bruno Cadonna <[email protected]>, Anna Sophie Blee-Goldman <[email protected]> commit 896af1b Author: Sanskar Jhajharia <[email protected]> Date: Thu Jun 6 01:46:59 2024 +0530 MINOR: Raft module Cleanup (apache#16205) Reviewers: Chia-Ping Tsai <[email protected]> commit 0109a3f Author: Antoine Pourchet <[email protected]> Date: Wed Jun 5 14:09:37 2024 -0600 KAFKA-15045: (KIP-924 pt. 17) State store computation fixed (apache#16194) Fixed the calculation of the store name list based on the subtopology being accessed. Also added a new test to make sure this new functionality works as intended. Reviewers: Anna Sophie Blee-Goldman <[email protected]> commit 52514a8 Author: Greg Harris <[email protected]> Date: Wed Jun 5 11:35:32 2024 -0700 KAFKA-16858: Throw DataException from validateValue on array and map schemas without inner schemas (apache#16161) Signed-off-by: Greg Harris <[email protected]> Reviewers: Chris Egerton <[email protected]> commit f2aafcc Author: Sanskar Jhajharia <[email protected]> Date: Wed Jun 5 20:06:01 2024 +0530 MINOR: Cleanups in Shell Module (apache#16204) Reviewers: Chia-Ping Tsai <[email protected]> commit bd9d68f Author: Abhijeet Kumar <[email protected]> Date: Wed Jun 5 19:12:25 2024 +0530 KAFKA-15265: Integrate RLMQuotaManager for throttling fetches from remote storage (apache#16071) Reviewers: Kamal Chandraprakash<[email protected]>, Luke Chen <[email protected]>, Satish Duggana <[email protected]> commit 62e5cce Author: gongxuanzhang <[email protected]> Date: Wed Jun 5 18:57:32 2024 +0800 KAFKA-10787 Update spotless version and remove support JDK8 (apache#16176) Reviewers: Chia-Ping Tsai <[email protected]> commit 02c794d Author: Kamal Chandraprakash <[email protected]> Date: Wed Jun 5 12:12:23 2024 +0530 KAFKA-15776: Introduce remote.fetch.max.timeout.ms to configure DelayedRemoteFetch timeout (apache#14778) KIP-1018, part1, Introduce remote.fetch.max.timeout.ms to configure DelayedRemoteFetch timeout Reviewers: Luke Chen <[email protected]> commit 7ddfa64 Author: Dongnuo Lyu <[email protected]> Date: Wed Jun 5 02:08:38 2024 -0400 MINOR: Adjust validateOffsetCommit/Fetch in ConsumerGroup to ensure compatibility with classic protocol members (apache#16145) During online migration, there could be ConsumerGroup that has members that uses the classic protocol. In the current implementation, `STALE_MEMBER_EPOCH` could be thrown in ConsumerGroup offset fetch/commit validation but it's not supported by the classic protocol. Thus this patch changed `ConsumerGroup#validateOffsetCommit` and `ConsumerGroup#validateOffsetFetch` to ensure compatibility. Reviewers: Jeff Kim <[email protected]>, David Jacot <[email protected]> commit 252c1ac Author: Apoorv Mittal <[email protected]> Date: Wed Jun 5 05:55:24 2024 +0100 KAFKA-16740: Adding skeleton code for Share Fetch and Acknowledge RPC (KIP-932) (apache#16184) The PR adds skeleton code for Share Fetch and Acknowledge RPCs. The changes include: 1. Defining RPCs in KafkaApis.scala 2. Added new SharePartitionManager class which handles the RPCs handling 3. Added SharePartition class which manages in-memory record states and for fetched data. Reviewers: David Jacot <[email protected]>, Andrew Schofield <[email protected]>, Manikumar Reddy <[email protected]> commit b89999b Author: PoAn Yang <[email protected]> Date: Wed Jun 5 08:02:52 2024 +0800 KAFKA-16483: Remove preAppendErrors from createPutCacheCallback (apache#16105) The method createPutCacheCallback has a input argument preAppendErrors. It is used to keep the "error" happens before appending. However, it is always empty. Also, the pre-append error is handled before createPutCacheCallback by calling responseCallback. Hence, we can remove preAppendErrors. Signed-off-by: PoAn Yang <[email protected]> Reviewers: Luke Chen <[email protected]> commit 01e9918 Author: Kuan-Po (Cooper) Tseng <[email protected]> Date: Wed Jun 5 07:56:18 2024 +0800 KAFKA-16814 KRaft broker cannot startup when `partition.metadata` is missing (apache#16165) When starting up kafka logManager, we'll check stray replicas to avoid some corner cases. But this check might cause broker unable to startup if partition.metadata is missing because when startup kafka, we load log from file, and the topicId of the log is coming from partition.metadata file. So, if partition.metadata is missing, the topicId will be None, and the LogManager#isStrayKraftReplica will fail with no topicID error. The partition.metadata missing could be some storage failure, or another possible path is unclean shutdown after topic is created in the replica, but before data is flushed into partition.metadata file. This is possible because we do the flush in async way here. When finding a log without topicID, we should treat it as a stray log and then delete it. Reviewers: Luke Chen <[email protected]>, Gaurav Narula <[email protected]> commit d652f5c Author: TingIāu "Ting" Kì <[email protected]> Date: Wed Jun 5 07:52:06 2024 +0800 MINOR: Add topicIds and directoryIds to the return value of the toString method. (apache#16189) Add topicIds and directoryIds to the return value of the toString method. Reviewers: Luke Chen <[email protected]> commit 7e0caad Author: Igor Soarez <[email protected]> Date: Tue Jun 4 22:12:33 2024 +0100 MINOR: Cleanup unused references in core (apache#16192) Reviewers: Chia-Ping Tsai <[email protected]> commit 9821aca Author: PoAn Yang <[email protected]> Date: Wed Jun 5 05:09:04 2024 +0800 MINOR: Upgrade gradle from 8.7 to 8.8 (apache#16190) Reviewers: Chia-Ping Tsai <[email protected]> commit 9ceed8f Author: Colin P. McCabe <[email protected]> Date: Tue Jun 4 14:04:59 2024 -0700 KAFKA-16535: Implement AddVoter, RemoveVoter, UpdateVoter RPCs Implement the add voter, remove voter, and update voter RPCs for KIP-853. This is just adding the RPC handling; the current implementation in RaftManager just throws UnsupportedVersionException. Reviewers: Andrew Schofield <[email protected]>, José Armando García Sancio <[email protected]> commit 8b3c77c Author: TingIāu "Ting" Kì <[email protected]> Date: Wed Jun 5 04:21:20 2024 +0800 KAFKA-15305 The background thread should try to process the remaining task until the shutdown timer is expired. (apache#16156) Reviewers: Lianet Magrans <[email protected]>, Chia-Ping Tsai <[email protected]> commit cda2df5 Author: Kamal Chandraprakash <[email protected]> Date: Wed Jun 5 00:41:30 2024 +0530 KAFKA-16882 Migrate RemoteLogSegmentLifecycleTest to ClusterInstance infra (apache#16180) - Removed the RemoteLogSegmentLifecycleManager - Removed the TopicBasedRemoteLogMetadataManagerWrapper, RemoteLogMetadataCacheWrapper, TopicBasedRemoteLogMetadataManagerHarness and TopicBasedRemoteLogMetadataManagerWrapperWithHarness Reviewers: Kuan-Po (Cooper) Tseng <[email protected]>, Chia-Ping Tsai <[email protected]> commit 2b47798 Author: Chris Egerton <[email protected]> Date: Tue Jun 4 21:04:34 2024 +0200 MINOR: Fix return tag on Javadocs for consumer group-related Admin methods (apache#16197) Reviewers: Greg Harris <[email protected]>, Chia-Ping Tsai <[email protected]>

muralibasani · 2024-06-07T12:24:01Z

We want to update the below configs dynamically:

remote.log.manager.copier.thread.pool.size

remote.log.manager.expiration.thread.pool.size

remote.log.manager.thread.pool.size and

remote.fetch.max.wait.ms

Dynamic values gets reflected in two ways:

A scheduler which checks the value (kafkaConfig.remoteLogManagerConfig.xyz) and performs an action. Once the config gets updated dynamically, it takes effect in the next run.

Continuous running job, once the config is updated we have to invoke those respective components to take effect the updated config change.

How to support dynamic configs for case-1 via this class? KafkaConfig has a nice abstraction of currentConfig which handles the dynamic config. One way to do is to move the getter methods from RLMConfig to KafkaConfig to reflect the updated values.

Would like to get your thoughts on this. We can take this up separately.

@kamalcph I was going through DynamicBrokerConfig.scala and found this
object DynamicRemoteLogConfig {
val ReconfigurableConfigs = Set(
RemoteLogManagerConfig.REMOTE_LOG_INDEX_FILE_CACHE_TOTAL_SIZE_BYTES_PROP
)
}

probably we can take this existing approach ? And better to have a jira ticket, so it can be discussed openly there.

…he#16199) Reviewers: Greg Harris <[email protected]>, Kamal Chandraprakash <[email protected]>, Chia-Ping Tsai <[email protected]>

Reviewers: Greg Harris <[email protected]>, Kamal Chandraprakash <[email protected]>, Chia-Ping Tsai <[email protected]>

kamalcph · 2024-06-11T16:55:06Z

We want to update the below configs dynamically:

remote.log.manager.copier.thread.pool.size

remote.log.manager.expiration.thread.pool.size

remote.log.manager.thread.pool.size and

remote.fetch.max.wait.ms

Dynamic values gets reflected in two ways:

A scheduler which checks the value (kafkaConfig.remoteLogManagerConfig.xyz) and performs an action. Once the config gets updated dynamically, it takes effect in the next run.

Continuous running job, once the config is updated we have to invoke those respective components to take effect the updated config change.

How to support dynamic configs for case-1 via this class? KafkaConfig has a nice abstraction of currentConfig which handles the dynamic config. One way to do is to move the getter methods from RLMConfig to KafkaConfig to reflect the updated values.
Would like to get your thoughts on this. We can take this up separately.

@kamalcph I was going through DynamicBrokerConfig.scala and found this object DynamicRemoteLogConfig { val ReconfigurableConfigs = Set( RemoteLogManagerConfig.REMOTE_LOG_INDEX_FILE_CACHE_TOTAL_SIZE_BYTES_PROP ) }

probably we can take this existing approach ? And better to have a jira ticket, so it can be discussed openly there.

Went with similar approach as discussed in #16203.

…he#16199) Reviewers: Greg Harris <[email protected]>, Kamal Chandraprakash <[email protected]>, Chia-Ping Tsai <[email protected]>

gharris1727 reviewed Jun 4, 2024

View reviewed changes

muralibasani requested a review from gharris1727 June 4, 2024 20:06

gharris1727 reviewed Jun 4, 2024

View reviewed changes

gharris1727 added the tiered-storage Related to the Tiered Storage feature label Jun 4, 2024

kamalcph self-requested a review June 5, 2024 04:08

kamalcph mentioned this pull request Jun 5, 2024

KAFKA-15776: Support added to update remote.fetch.max.wait.ms dynamically #16203

Merged

3 tasks

kamalcph reviewed Jun 5, 2024

View reviewed changes

storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfig.java Show resolved Hide resolved

muralibasani added 3 commits June 5, 2024 16:35

Refactor RemoteLogManagerConfig with AbstractConfig

0c296a1

Refactor RemoteLogManagerConfig with AbstractConfig

1d0d831

Refactor RemoteLogManagerConfig with AbstractConfig

4344ed6

muralibasani force-pushed the KAFKA-16884 branch from 3839b4e to 4344ed6 Compare June 5, 2024 14:40

muralibasani added 2 commits June 5, 2024 17:06

Fix from review

9e69e8b

Fix from review

208a254

gharris1727 reviewed Jun 5, 2024

View reviewed changes

muralibasani added 2 commits June 5, 2024 18:25

Add empty string test

75fe888

Updating configdef

f035978

muralibasani requested a review from gharris1727 June 5, 2024 16:33

Removing CONFIG_DEF

7b9809a

gharris1727 reviewed Jun 5, 2024

View reviewed changes

...age/src/test/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfigTest.java Outdated Show resolved Hide resolved

...age/src/test/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfigTest.java Outdated Show resolved Hide resolved

gharris1727 reviewed Jun 5, 2024

View reviewed changes

...age/src/test/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfigTest.java Show resolved Hide resolved

Splitting tests

4f36cdd

muralibasani requested a review from gharris1727 June 5, 2024 18:30

Adding another assertion

f89c240

gharris1727 approved these changes Jun 5, 2024

View reviewed changes

chia7712 reviewed Jun 5, 2024

View reviewed changes

Removing unnecessary unmodifiableMap call

f67a734

muralibasani requested review from chia7712 and kamalcph June 5, 2024 20:30

chia7712 reviewed Jun 5, 2024

View reviewed changes

update condition for empty map

81a6966

chia7712 approved these changes Jun 5, 2024

View reviewed changes

kamalcph reviewed Jun 6, 2024

View reviewed changes

Fix tests from review

a70391f

kamalcph approved these changes Jun 6, 2024

View reviewed changes

chia7712 merged commit a41f7a4 into apache:trunk Jun 6, 2024
1 check failed

satishd pushed a commit that referenced this pull request Jun 11, 2024

KAFKA-16884 Refactor RemoteLogManagerConfig with AbstractConfig (#16199)

9460e6b

Reviewers: Greg Harris <[email protected]>, Kamal Chandraprakash <[email protected]>, Chia-Ping Tsai <[email protected]>

kamalcph mentioned this pull request Jun 19, 2024

KAFKA-16976: Update the current/dynamic config inside RemoteLogManagerConfig #16394

Merged

3 tasks

KAFKA-16884 : Refactor RemoteLogManagerConfig with AbstractConfig #16199

KAFKA-16884 : Refactor RemoteLogManagerConfig with AbstractConfig #16199

Conversation

muralibasani commented Jun 4, 2024

Committer Checklist (excluded from commit message)

muralibasani commented Jun 4, 2024

gharris1727 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gharris1727 Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamalcph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muralibasani Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

gharris1727 Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

gharris1727 left a comment

Choose a reason for hiding this comment

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chia7712 left a comment

Choose a reason for hiding this comment

kamalcph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muralibasani Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

kamalcph commented Jun 6, 2024 • edited Loading

kamalcph left a comment

Choose a reason for hiding this comment

muralibasani commented Jun 6, 2024

chia7712 commented Jun 6, 2024

muralibasani commented Jun 7, 2024

kamalcph commented Jun 11, 2024

gharris1727 Jun 5, 2024 •

edited

Loading

muralibasani Jun 5, 2024 •

edited

Loading

gharris1727 Jun 5, 2024 •

edited

Loading

muralibasani Jun 6, 2024 •

edited

Loading

kamalcph commented Jun 6, 2024 •

edited

Loading