You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/07/22 19:30:47 UTC
[GitHub] [kafka] ccding opened a new pull request #11110: MINOR: move tiered storage related configs to a separate class within LogConfig
ccding opened a new pull request #11110:
URL: https://github.com/apache/kafka/pull/11110
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ccding commented on a change in pull request #11110: MINOR: move tiered storage related configs to a separate class within LogConfig
Posted by GitBox <gi...@apache.org>.
ccding commented on a change in pull request #11110:
URL: https://github.com/apache/kafka/pull/11110#discussion_r683573626
##########
File path: storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfig.java
##########
@@ -253,9 +253,9 @@ public RemoteLogManagerConfig(AbstractConfig config) {
config.getInt(REMOTE_LOG_READER_THREADS_PROP),
config.getInt(REMOTE_LOG_READER_MAX_PENDING_TASKS_PROP),
config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP),
- config.originalsWithPrefix(config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP)),
+ config.originalsWithPrefix(config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP) == null ? "" : config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP)),
Review comment:
We will get a null pointer exception everywhere when we call `new KafkaConfig()` without setting `REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP`. E.g., https://github.com/apache/kafka/blob/d6f6edd2b17cbaccd4c87bfa66f871a676760044/core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala#L1057
This happens everywhere in the codebase.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] satishd commented on a change in pull request #11110: MINOR: move tiered storage related configs to a separate class within LogConfig
Posted by GitBox <gi...@apache.org>.
satishd commented on a change in pull request #11110:
URL: https://github.com/apache/kafka/pull/11110#discussion_r683370891
##########
File path: storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfig.java
##########
@@ -253,9 +253,9 @@ public RemoteLogManagerConfig(AbstractConfig config) {
config.getInt(REMOTE_LOG_READER_THREADS_PROP),
config.getInt(REMOTE_LOG_READER_MAX_PENDING_TASKS_PROP),
config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP),
- config.originalsWithPrefix(config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP)),
+ config.originalsWithPrefix(config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP) == null ? "" : config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP)),
Review comment:
What is the intent for this change here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ccding commented on a change in pull request #11110: MINOR: move tiered storage related configs to a separate class within LogConfig
Posted by GitBox <gi...@apache.org>.
ccding commented on a change in pull request #11110:
URL: https://github.com/apache/kafka/pull/11110#discussion_r682281792
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -1434,6 +1434,9 @@ class KafkaConfig(val props: java.util.Map[_, _], doLog: Boolean, dynamicConfigO
val zkEnableSecureAcls: Boolean = getBoolean(KafkaConfig.ZkEnableSecureAclsProp)
val zkMaxInFlightRequests: Int = getInt(KafkaConfig.ZkMaxInFlightRequestsProp)
+ private val _tieredKafkaConfig = new RemoteLogManagerConfig(this)
Review comment:
I had a hard time naming this variable. Would `_remoteLogManagerConfig` be better?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #11110: MINOR: move tiered storage related configs to a separate class within LogConfig
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #11110:
URL: https://github.com/apache/kafka/pull/11110#issuecomment-886113803
Thanks for the PR. We should include the motivation when suggesting a change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao merged pull request #11110: MINOR: move tiered storage related configs to a separate class within LogConfig
Posted by GitBox <gi...@apache.org>.
junrao merged pull request #11110:
URL: https://github.com/apache/kafka/pull/11110
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao commented on a change in pull request #11110: MINOR: move tiered storage related configs to a separate class within LogConfig
Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #11110:
URL: https://github.com/apache/kafka/pull/11110#discussion_r682820398
##########
File path: core/src/main/scala/kafka/log/LogConfig.scala
##########
@@ -107,46 +107,52 @@ case class LogConfig(props: java.util.Map[_, _], overriddenConfigs: Set[String]
val LeaderReplicationThrottledReplicas = getList(LogConfig.LeaderReplicationThrottledReplicasProp)
val FollowerReplicationThrottledReplicas = getList(LogConfig.FollowerReplicationThrottledReplicasProp)
val messageDownConversionEnable = getBoolean(LogConfig.MessageDownConversionEnableProp)
- val remoteStorageEnable = getBoolean(LogConfig.RemoteLogStorageEnableProp)
- val localRetentionMs: Long = {
- val localLogRetentionMs = getLong(LogConfig.LocalLogRetentionMsProp)
+ class TieredLogConfig {
Review comment:
This probably should be named RemoteLogConfig to match RemoteLogManagerConfig?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #11110: MINOR: move tiered storage related configs to a separate class within LogConfig
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #11110:
URL: https://github.com/apache/kafka/pull/11110#discussion_r682253716
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -1434,6 +1434,9 @@ class KafkaConfig(val props: java.util.Map[_, _], doLog: Boolean, dynamicConfigO
val zkEnableSecureAcls: Boolean = getBoolean(KafkaConfig.ZkEnableSecureAclsProp)
val zkMaxInFlightRequests: Int = getInt(KafkaConfig.ZkMaxInFlightRequestsProp)
+ private val _tieredKafkaConfig = new RemoteLogManagerConfig(this)
Review comment:
Why is this prefixed with `tiered` while the class name is prefixed with `Remote`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ccding commented on a change in pull request #11110: MINOR: move tiered storage related configs to a separate class within LogConfig
Posted by GitBox <gi...@apache.org>.
ccding commented on a change in pull request #11110:
URL: https://github.com/apache/kafka/pull/11110#discussion_r683068699
##########
File path: core/src/main/scala/kafka/log/LogConfig.scala
##########
@@ -107,46 +107,52 @@ case class LogConfig(props: java.util.Map[_, _], overriddenConfigs: Set[String]
val LeaderReplicationThrottledReplicas = getList(LogConfig.LeaderReplicationThrottledReplicasProp)
val FollowerReplicationThrottledReplicas = getList(LogConfig.FollowerReplicationThrottledReplicasProp)
val messageDownConversionEnable = getBoolean(LogConfig.MessageDownConversionEnableProp)
- val remoteStorageEnable = getBoolean(LogConfig.RemoteLogStorageEnableProp)
- val localRetentionMs: Long = {
- val localLogRetentionMs = getLong(LogConfig.LocalLogRetentionMsProp)
+ class TieredLogConfig {
Review comment:
Done. Thanks for the comment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ccding commented on pull request #11110: MINOR: move tiered storage related configs to a separate class within LogConfig
Posted by GitBox <gi...@apache.org>.
ccding commented on pull request #11110:
URL: https://github.com/apache/kafka/pull/11110#issuecomment-892299485
Included the motivation and updated the PR. PTAL @ijuma @junrao @kowshik @satishd
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kowshik commented on a change in pull request #11110: MINOR: move tiered storage related configs to a separate class within LogConfig
Posted by GitBox <gi...@apache.org>.
kowshik commented on a change in pull request #11110:
URL: https://github.com/apache/kafka/pull/11110#discussion_r683184053
##########
File path: storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfig.java
##########
@@ -253,9 +253,9 @@ public RemoteLogManagerConfig(AbstractConfig config) {
config.getInt(REMOTE_LOG_READER_THREADS_PROP),
config.getInt(REMOTE_LOG_READER_MAX_PENDING_TASKS_PROP),
config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP),
- config.originalsWithPrefix(config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP)),
+ config.originalsWithPrefix(config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP) == null ? "" : config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP)),
Review comment:
Hmm, why is this change needed? It doesn't seem like the PR is altering behavior such as these but maybe I'm missing something.
##########
File path: core/src/main/scala/kafka/log/LogConfig.scala
##########
@@ -107,46 +107,52 @@ case class LogConfig(props: java.util.Map[_, _], overriddenConfigs: Set[String]
val LeaderReplicationThrottledReplicas = getList(LogConfig.LeaderReplicationThrottledReplicasProp)
val FollowerReplicationThrottledReplicas = getList(LogConfig.FollowerReplicationThrottledReplicasProp)
val messageDownConversionEnable = getBoolean(LogConfig.MessageDownConversionEnableProp)
- val remoteStorageEnable = getBoolean(LogConfig.RemoteLogStorageEnableProp)
- val localRetentionMs: Long = {
- val localLogRetentionMs = getLong(LogConfig.LocalLogRetentionMsProp)
+ class RemoteLogConfig {
+ val remoteStorageEnable = getBoolean(LogConfig.RemoteLogStorageEnableProp)
- // -2 indicates to derive value from retentionMs property.
- if(localLogRetentionMs == -2) retentionMs
- else {
- // Added validation here to check the effective value should not be more than RetentionMs.
- if(localLogRetentionMs == -1 && retentionMs != -1) {
- throw new ConfigException(LogConfig.LocalLogRetentionMsProp, localLogRetentionMs, s"Value must not be -1 as ${LogConfig.RetentionMsProp} value is set as $retentionMs.")
- }
+ val localRetentionMs: Long = {
+ val localLogRetentionMs = getLong(LogConfig.LocalLogRetentionMsProp)
- if (localLogRetentionMs > retentionMs) {
- throw new ConfigException(LogConfig.LocalLogRetentionMsProp, localLogRetentionMs, s"Value must not be more than property: ${LogConfig.RetentionMsProp} value.")
- }
+ // -2 indicates to derive value from retentionMs property.
+ if(localLogRetentionMs == -2) retentionMs
+ else {
+ // Added validation here to check the effective value should not be more than RetentionMs.
+ if(localLogRetentionMs == -1 && retentionMs != -1) {
+ throw new ConfigException(LogConfig.LocalLogRetentionMsProp, localLogRetentionMs, s"Value must not be -1 as ${LogConfig.RetentionMsProp} value is set as $retentionMs.")
+ }
+
+ if (localLogRetentionMs > retentionMs) {
+ throw new ConfigException(LogConfig.LocalLogRetentionMsProp, localLogRetentionMs, s"Value must not be more than property: ${LogConfig.RetentionMsProp} value.")
+ }
- localLogRetentionMs
+ localLogRetentionMs
+ }
}
- }
- val localRetentionBytes: Long = {
- val localLogRetentionBytes = getLong(LogConfig.LocalLogRetentionBytesProp)
+ val localRetentionBytes: Long = {
+ val localLogRetentionBytes = getLong(LogConfig.LocalLogRetentionBytesProp)
- // -2 indicates to derive value from retentionSize property.
- if(localLogRetentionBytes == -2) retentionSize;
- else {
- // Added validation here to check the effective value should not be more than RetentionBytes.
- if(localLogRetentionBytes == -1 && retentionSize != -1) {
- throw new ConfigException(LogConfig.LocalLogRetentionBytesProp, localLogRetentionBytes, s"Value must not be -1 as ${LogConfig.RetentionBytesProp} value is set as $retentionSize.")
- }
+ // -2 indicates to derive value from retentionSize property.
+ if(localLogRetentionBytes == -2) retentionSize;
Review comment:
nit: it seems like we could remove the semicolon at the end.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ccding commented on pull request #11110: MINOR: move tiered storage related configs to a separate class within LogConfig
Posted by GitBox <gi...@apache.org>.
ccding commented on pull request #11110:
URL: https://github.com/apache/kafka/pull/11110#issuecomment-906457166
Thank you @satishd
Addressed your comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] satishd commented on a change in pull request #11110: MINOR: move tiered storage related configs to a separate class within LogConfig
Posted by GitBox <gi...@apache.org>.
satishd commented on a change in pull request #11110:
URL: https://github.com/apache/kafka/pull/11110#discussion_r696454031
##########
File path: storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfig.java
##########
@@ -253,9 +253,9 @@ public RemoteLogManagerConfig(AbstractConfig config) {
config.getInt(REMOTE_LOG_READER_THREADS_PROP),
config.getInt(REMOTE_LOG_READER_MAX_PENDING_TASKS_PROP),
config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP),
- config.originalsWithPrefix(config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP)),
+ config.originalsWithPrefix(config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP) == null ? "" : config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP)),
Review comment:
@ccding It should be non empty based on the validator that is set for this config. If an empty string is passed then all the Kafka config properties will be passed, which is wrong.
There should be null validation instead of passing an empty string as mentioned below. Same for the `REMOTE_LOG_METADATA_MANAGER_CONFIG_PREFIX_PROP` check.
```
config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP) != null
? config.originalsWithPrefix(config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP))
: Collections.emptyMap()
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ccding commented on a change in pull request #11110: MINOR: move tiered storage related configs to a separate class within LogConfig
Posted by GitBox <gi...@apache.org>.
ccding commented on a change in pull request #11110:
URL: https://github.com/apache/kafka/pull/11110#discussion_r683573626
##########
File path: storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfig.java
##########
@@ -253,9 +253,9 @@ public RemoteLogManagerConfig(AbstractConfig config) {
config.getInt(REMOTE_LOG_READER_THREADS_PROP),
config.getInt(REMOTE_LOG_READER_MAX_PENDING_TASKS_PROP),
config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP),
- config.originalsWithPrefix(config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP)),
+ config.originalsWithPrefix(config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP) == null ? "" : config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP)),
Review comment:
We will get a null pointer exception when we call `new KafkaConfig()` without setting `REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP`. E.g., https://github.com/apache/kafka/blob/d6f6edd2b17cbaccd4c87bfa66f871a676760044/core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala#L1057
This happens everywhere in the codebase.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao commented on a change in pull request #11110: MINOR: move tiered storage related configs to a separate class within LogConfig
Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #11110:
URL: https://github.com/apache/kafka/pull/11110#discussion_r682820398
##########
File path: core/src/main/scala/kafka/log/LogConfig.scala
##########
@@ -107,46 +107,52 @@ case class LogConfig(props: java.util.Map[_, _], overriddenConfigs: Set[String]
val LeaderReplicationThrottledReplicas = getList(LogConfig.LeaderReplicationThrottledReplicasProp)
val FollowerReplicationThrottledReplicas = getList(LogConfig.FollowerReplicationThrottledReplicasProp)
val messageDownConversionEnable = getBoolean(LogConfig.MessageDownConversionEnableProp)
- val remoteStorageEnable = getBoolean(LogConfig.RemoteLogStorageEnableProp)
- val localRetentionMs: Long = {
- val localLogRetentionMs = getLong(LogConfig.LocalLogRetentionMsProp)
+ class TieredLogConfig {
Review comment:
This probably should be named RemoteLogConfig to match RemoteLogManagerConfig?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ccding commented on a change in pull request #11110: MINOR: move tiered storage related configs to a separate class within LogConfig
Posted by GitBox <gi...@apache.org>.
ccding commented on a change in pull request #11110:
URL: https://github.com/apache/kafka/pull/11110#discussion_r683573626
##########
File path: storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfig.java
##########
@@ -253,9 +253,9 @@ public RemoteLogManagerConfig(AbstractConfig config) {
config.getInt(REMOTE_LOG_READER_THREADS_PROP),
config.getInt(REMOTE_LOG_READER_MAX_PENDING_TASKS_PROP),
config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP),
- config.originalsWithPrefix(config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP)),
+ config.originalsWithPrefix(config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP) == null ? "" : config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP)),
Review comment:
We will get a null pointer exception when we call `new KafkaConfig()` without setting `REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP`. E.g., https://github.com/apache/kafka/blob/d6f6edd2b17cbaccd4c87bfa66f871a676760044/core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala#L1057
This happens everywhere in the codebase after this change:
https://github.com/apache/kafka/pull/11110/files#diff-cbe6a8b71b05ed22cf09d97591225b588e9fca6caaf95d3b34a43262cfd23aa6R1437-R1438
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #11110: MINOR: move tiered storage related configs to a separate class within LogConfig
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #11110:
URL: https://github.com/apache/kafka/pull/11110#issuecomment-893407158
Thanks for explaining the motivation @ccding. In my opinion, this is a bit confusing. What makes remote log configs special when compared to local log configs? The same arguments regarding reuse, etc. could be said for those, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] satishd commented on pull request #11110: MINOR: move tiered storage related configs to a separate class within LogConfig
Posted by GitBox <gi...@apache.org>.
satishd commented on pull request #11110:
URL: https://github.com/apache/kafka/pull/11110#issuecomment-906243973
@junrao The intention of the PR looks reasonable to me. I left a comment [here](https://github.com/apache/kafka/pull/11110#discussion_r696454031) on one of the changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ccding commented on a change in pull request #11110: MINOR: move tiered storage related configs to a separate class within LogConfig
Posted by GitBox <gi...@apache.org>.
ccding commented on a change in pull request #11110:
URL: https://github.com/apache/kafka/pull/11110#discussion_r683573626
##########
File path: storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfig.java
##########
@@ -253,9 +253,9 @@ public RemoteLogManagerConfig(AbstractConfig config) {
config.getInt(REMOTE_LOG_READER_THREADS_PROP),
config.getInt(REMOTE_LOG_READER_MAX_PENDING_TASKS_PROP),
config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP),
- config.originalsWithPrefix(config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP)),
+ config.originalsWithPrefix(config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP) == null ? "" : config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP)),
Review comment:
We will get a null pointer exception when we call `new KafkaConfig()` without setting `REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP`. E.g., https://github.com/apache/kafka/blob/d6f6edd2b17cbaccd4c87bfa66f871a676760044/core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala#L1057
This happens everywhere in the codebase after this change:
https://github.com/apache/kafka/pull/11110/files#diff-cbe6a8b71b05ed22cf09d97591225b588e9fca6caaf95d3b34a43262cfd23aa6R1437-R1438
Since `REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP` is not a required configuration if we don't enable KIP-405, I think we should pass an empty string to RemoteLogManagerConfig if it is null.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ccding commented on a change in pull request #11110: MINOR: move tiered storage related configs to a separate class within LogConfig
Posted by GitBox <gi...@apache.org>.
ccding commented on a change in pull request #11110:
URL: https://github.com/apache/kafka/pull/11110#discussion_r683068699
##########
File path: core/src/main/scala/kafka/log/LogConfig.scala
##########
@@ -107,46 +107,52 @@ case class LogConfig(props: java.util.Map[_, _], overriddenConfigs: Set[String]
val LeaderReplicationThrottledReplicas = getList(LogConfig.LeaderReplicationThrottledReplicasProp)
val FollowerReplicationThrottledReplicas = getList(LogConfig.FollowerReplicationThrottledReplicasProp)
val messageDownConversionEnable = getBoolean(LogConfig.MessageDownConversionEnableProp)
- val remoteStorageEnable = getBoolean(LogConfig.RemoteLogStorageEnableProp)
- val localRetentionMs: Long = {
- val localLogRetentionMs = getLong(LogConfig.LocalLogRetentionMsProp)
+ class TieredLogConfig {
Review comment:
Done. Thanks for the comment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kowshik commented on a change in pull request #11110: MINOR: move tiered storage related configs to a separate class within LogConfig
Posted by GitBox <gi...@apache.org>.
kowshik commented on a change in pull request #11110:
URL: https://github.com/apache/kafka/pull/11110#discussion_r683184053
##########
File path: storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfig.java
##########
@@ -253,9 +253,9 @@ public RemoteLogManagerConfig(AbstractConfig config) {
config.getInt(REMOTE_LOG_READER_THREADS_PROP),
config.getInt(REMOTE_LOG_READER_MAX_PENDING_TASKS_PROP),
config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP),
- config.originalsWithPrefix(config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP)),
+ config.originalsWithPrefix(config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP) == null ? "" : config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP)),
Review comment:
Hmm, why is this change needed? It doesn't seem like the PR is altering behavior such as these but maybe I'm missing something.
##########
File path: core/src/main/scala/kafka/log/LogConfig.scala
##########
@@ -107,46 +107,52 @@ case class LogConfig(props: java.util.Map[_, _], overriddenConfigs: Set[String]
val LeaderReplicationThrottledReplicas = getList(LogConfig.LeaderReplicationThrottledReplicasProp)
val FollowerReplicationThrottledReplicas = getList(LogConfig.FollowerReplicationThrottledReplicasProp)
val messageDownConversionEnable = getBoolean(LogConfig.MessageDownConversionEnableProp)
- val remoteStorageEnable = getBoolean(LogConfig.RemoteLogStorageEnableProp)
- val localRetentionMs: Long = {
- val localLogRetentionMs = getLong(LogConfig.LocalLogRetentionMsProp)
+ class RemoteLogConfig {
+ val remoteStorageEnable = getBoolean(LogConfig.RemoteLogStorageEnableProp)
- // -2 indicates to derive value from retentionMs property.
- if(localLogRetentionMs == -2) retentionMs
- else {
- // Added validation here to check the effective value should not be more than RetentionMs.
- if(localLogRetentionMs == -1 && retentionMs != -1) {
- throw new ConfigException(LogConfig.LocalLogRetentionMsProp, localLogRetentionMs, s"Value must not be -1 as ${LogConfig.RetentionMsProp} value is set as $retentionMs.")
- }
+ val localRetentionMs: Long = {
+ val localLogRetentionMs = getLong(LogConfig.LocalLogRetentionMsProp)
- if (localLogRetentionMs > retentionMs) {
- throw new ConfigException(LogConfig.LocalLogRetentionMsProp, localLogRetentionMs, s"Value must not be more than property: ${LogConfig.RetentionMsProp} value.")
- }
+ // -2 indicates to derive value from retentionMs property.
+ if(localLogRetentionMs == -2) retentionMs
+ else {
+ // Added validation here to check the effective value should not be more than RetentionMs.
+ if(localLogRetentionMs == -1 && retentionMs != -1) {
+ throw new ConfigException(LogConfig.LocalLogRetentionMsProp, localLogRetentionMs, s"Value must not be -1 as ${LogConfig.RetentionMsProp} value is set as $retentionMs.")
+ }
+
+ if (localLogRetentionMs > retentionMs) {
+ throw new ConfigException(LogConfig.LocalLogRetentionMsProp, localLogRetentionMs, s"Value must not be more than property: ${LogConfig.RetentionMsProp} value.")
+ }
- localLogRetentionMs
+ localLogRetentionMs
+ }
}
- }
- val localRetentionBytes: Long = {
- val localLogRetentionBytes = getLong(LogConfig.LocalLogRetentionBytesProp)
+ val localRetentionBytes: Long = {
+ val localLogRetentionBytes = getLong(LogConfig.LocalLogRetentionBytesProp)
- // -2 indicates to derive value from retentionSize property.
- if(localLogRetentionBytes == -2) retentionSize;
- else {
- // Added validation here to check the effective value should not be more than RetentionBytes.
- if(localLogRetentionBytes == -1 && retentionSize != -1) {
- throw new ConfigException(LogConfig.LocalLogRetentionBytesProp, localLogRetentionBytes, s"Value must not be -1 as ${LogConfig.RetentionBytesProp} value is set as $retentionSize.")
- }
+ // -2 indicates to derive value from retentionSize property.
+ if(localLogRetentionBytes == -2) retentionSize;
Review comment:
nit: it seems like we could remove the semicolon at the end.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org