KAFKA-15083: add config with "remote.log.metadata" prefix #14151

showuon · 2023-08-04T10:06:08Z

When configuring RLMM, the configs passed into configure method is the RemoteLogManagerConfig. But in RemoteLogManagerConfig, there's no configs related to remote.log.metadata.*, ex: remote.log.metadata.topic.replication.factor. So, even if users have set the config in broker, it'll never be applied.

This PR fixed the issue to allow users setting RLMM prefix: remote.log.metadata.manager.impl.prefix (default is rlmm.config.), and then, appending the desired remote.log.metadata.* configs, it'll pass into RLMM, including remote.log.metadata.common.client./remote.log.metadata.producer./ remote.log.metadata.consumer. prefixes.

Ex:

# default value
# remote.log.storage.manager.impl.prefix=rsm.config.
# remote.log.metadata.manager.impl.prefix=rlmm.config.

rlmm.config.remote.log.metadata.topic.num.partitions=50
rlmm.config.remote.log.metadata.topic.replication.factor=4

rsm.config.test=value

Committer Checklist (excluded from commit message)

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

clolov

Yeah, okay, this pull request makes sense to me. To summarise, you are saying that configurations starting with remote.log.metadata need to be passed to implementations of the RemoteLogMetadataManager as part of the rlmmProps, however, they are never put in the rlmmProps by the RemoteLogManagerConfig?

Test failures appear to be unrelated.

showuon · 2023-08-05T04:57:55Z

Yeah, okay, this pull request makes sense to me. To summarise, you are saying that configurations starting with remote.log.metadata need to be passed to implementations of the RemoteLogMetadataManager as part of the rlmmProps, however, they are never put in the rlmmProps by the RemoteLogManagerConfig?

Test failures appear to be unrelated.

Correct! Thanks.

kamalcph

LGTM, I noticed the same while working on the test framework. Thanks for adding it!

kamalcph · 2023-08-05T05:49:12Z

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

@@ -134,6 +134,8 @@ public final class RemoteLogManagerConfig {
            "less than or equal to `log.retention.bytes` value.";
    public static final Long DEFAULT_LOG_LOCAL_RETENTION_BYTES = -2L;

+    public static final String REMOTE_LOG_METADATA_PREFIX = "remote.log.metadata";


Should we have append the _PROP (or) _CONFIG suffix to it?

We already have REMOTE_LOG_METADATA_MANAGER_CONFIG_PREFIX_PROP. Configs that are prefixed with this value will be supplied to remote log metadata manager.

To configure the values in the JIRA ticket:

remote.log.storage.manager.impl.prefix=remote.log.storage. remote.log.metadata.manager.impl.prefix=rlmm.config. rlmm.config.remote.log.metadata.topic.num.partitions=50 rlmm.config.remote.log.metadata.topic.replication.factor=4 rlmm.config.remote.log.metadata.topic.retention.ms=2592000000 remote.log.storage.s3...

Let's avoid one more Map (remoteLogMetadataProps) in RemoteLogManagerConfig.

Also, we can define the default values for the config prefix in this PR:

public static final String DEFAULT_REMOTE_STORAGE_MANAGER_CONFIG_PREFIX = "rsm.config."; public static final String DEFAULT_REMOTE_LOG_METADATA_MANAGER_CONFIG_PREFIX = "rlmm.config.";

Thanks for the good suggestion! PR updated.

kamalcph · 2023-08-05T06:04:44Z

Not clear on this patch, will go another round of review.

kamalcph · 2023-08-05T10:41:19Z

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

@@ -134,6 +134,8 @@ public final class RemoteLogManagerConfig {
            "less than or equal to `log.retention.bytes` value.";
    public static final Long DEFAULT_LOG_LOCAL_RETENTION_BYTES = -2L;

+    public static final String REMOTE_LOG_METADATA_PREFIX = "remote.log.metadata";


We already have REMOTE_LOG_METADATA_MANAGER_CONFIG_PREFIX_PROP. Configs that are prefixed with this value will be supplied to remote log metadata manager.

To configure the values in the JIRA ticket:

remote.log.storage.manager.impl.prefix=remote.log.storage. remote.log.metadata.manager.impl.prefix=rlmm.config. rlmm.config.remote.log.metadata.topic.num.partitions=50 rlmm.config.remote.log.metadata.topic.replication.factor=4 rlmm.config.remote.log.metadata.topic.retention.ms=2592000000 remote.log.storage.s3...

Let's avoid one more Map (remoteLogMetadataProps) in RemoteLogManagerConfig.

Also, we can define the default values for the config prefix in this PR:

public static final String DEFAULT_REMOTE_STORAGE_MANAGER_CONFIG_PREFIX = "rsm.config."; public static final String DEFAULT_REMOTE_LOG_METADATA_MANAGER_CONFIG_PREFIX = "rlmm.config.";

core/src/test/java/kafka/log/remote/RemoteLogManagerTest.java

kamalcph

LGTM

showuon · 2023-08-11T02:35:46Z

Failed tests are unrelated:

    Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationBaseTest.testOneWayReplicationWithAutoOffsetSync()
    Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationExactlyOnceTest.testOffsetTranslationBehindReplicationFlow()
    Build / JDK 11 and Scala 2.13 / kafka.api.TransactionsTest.testBumpTransactionalEpoch(String).quorum=kraft
    Build / JDK 11 and Scala 2.13 / kafka.log.remote.RemoteIndexCacheTest.testClose()
    Build / JDK 11 and Scala 2.13 / org.apache.kafka.controller.QuorumControllerTest.testBalancePartitionLeaders()
    Build / JDK 11 and Scala 2.13 / org.apache.kafka.controller.QuorumControllerTest.testConfigResourceExistenceChecker()
    Build / JDK 20 and Scala 2.13 / org.apache.kafka.controller.QuorumControllerTest.testBalancePartitionLeaders()
    Build / JDK 17 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationExactlyOnceTest.testOffsetTranslationBehindReplicationFlow()
    Build / JDK 17 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationSSLTest.testOffsetTranslationBehindReplicationFlow()
    Build / JDK 17 and Scala 2.13 / org.apache.kafka.controller.QuorumControllerTest.testBalancePartitionLeaders()

When configuring RLMM, the configs passed into configure method is the RemoteLogManagerConfig. But in RemoteLogManagerConfig, there's no configs related to remote.log.metadata.*, ex: remote.log.metadata.topic.replication.factor. So, even if users have set the config in broker, it'll never be applied. This PR fixed the issue to allow users setting RLMM prefix: remote.log.metadata.manager.impl.prefix (default is rlmm.config.), and then, appending the desired remote.log.metadata.* configs, it'll pass into RLMM, including remote.log.metadata.common.client./remote.log.metadata.producer./ remote.log.metadata.consumer. prefixes. Ex: rlmm.config.remote.log.metadata.topic.num.partitions=50 rlmm.config.remote.log.metadata.topic.replication.factor=4 rsm.config.test=value Reviewers: Christo Lolov <christololov@gmail.com>, Kamal Chandraprakash <kchandraprakash@uber.com>, Divij Vaidya <diviv@amazon.com>

When configuring RLMM, the configs passed into configure method is the RemoteLogManagerConfig. But in RemoteLogManagerConfig, there's no configs related to remote.log.metadata.*, ex: remote.log.metadata.topic.replication.factor. So, even if users have set the config in broker, it'll never be applied. This PR fixed the issue to allow users setting RLMM prefix: remote.log.metadata.manager.impl.prefix (default is rlmm.config.), and then, appending the desired remote.log.metadata.* configs, it'll pass into RLMM, including remote.log.metadata.common.client./remote.log.metadata.producer./ remote.log.metadata.consumer. prefixes. Ex: # default value # remote.log.storage.manager.impl.prefix=rsm.config. # remote.log.metadata.manager.impl.prefix=rlmm.config. rlmm.config.remote.log.metadata.topic.num.partitions=50 rlmm.config.remote.log.metadata.topic.replication.factor=4 rsm.config.test=value Reviewers: Christo Lolov <christololov@gmail.com>, Kamal Chandraprakash <kchandraprakash@uber.com>, Divij Vaidya <diviv@amazon.com>

KAFKA-15083: add config with "remote.log.metadata" prefix

4f4d5a8

showuon added the tiered-storage Pull requests associated with KIP-405 (Tiered Storage) label Aug 4, 2023

clolov approved these changes Aug 4, 2023

View reviewed changes

kamalcph approved these changes Aug 5, 2023

View reviewed changes

kamalcph reviewed Aug 5, 2023

View reviewed changes

kamalcph suggested changes Aug 5, 2023

View reviewed changes

divijvaidya reviewed Aug 9, 2023

View reviewed changes

core/src/test/java/kafka/log/remote/RemoteLogManagerTest.java Outdated Show resolved Hide resolved

showuon added 2 commits August 10, 2023 17:06

Merge branch 'trunk' of https://github.com/apache/kafka into KAFKA-15083

004dc2d

KAFKA-15083: set default RLS and RLMM config prefix

4b8ebd8

kamalcph approved these changes Aug 10, 2023

View reviewed changes

divijvaidya approved these changes Aug 10, 2023

View reviewed changes

showuon merged commit cdbc9a8 into apache:trunk Aug 11, 2023
1 check was pending

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-15083: add config with "remote.log.metadata" prefix #14151

KAFKA-15083: add config with "remote.log.metadata" prefix #14151

showuon commented Aug 4, 2023 •

edited

clolov left a comment •

edited

showuon commented Aug 5, 2023

kamalcph left a comment

kamalcph Aug 5, 2023 •

edited

kamalcph Aug 5, 2023

showuon Aug 10, 2023

kamalcph commented Aug 5, 2023

kamalcph Aug 5, 2023

kamalcph left a comment

showuon commented Aug 11, 2023

KAFKA-15083: add config with "remote.log.metadata" prefix #14151

KAFKA-15083: add config with "remote.log.metadata" prefix #14151

Conversation

showuon commented Aug 4, 2023 • edited

Committer Checklist (excluded from commit message)

clolov left a comment • edited

Choose a reason for hiding this comment

showuon commented Aug 5, 2023

kamalcph left a comment

Choose a reason for hiding this comment

kamalcph Aug 5, 2023 • edited

Choose a reason for hiding this comment

kamalcph Aug 5, 2023

Choose a reason for hiding this comment

showuon Aug 10, 2023

Choose a reason for hiding this comment

kamalcph commented Aug 5, 2023

kamalcph Aug 5, 2023

Choose a reason for hiding this comment

kamalcph left a comment

Choose a reason for hiding this comment

showuon commented Aug 11, 2023

showuon commented Aug 4, 2023 •

edited

clolov left a comment •

edited

kamalcph Aug 5, 2023 •

edited