Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
OmniaGM commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1466303341
##
server/src/main/java/org/apache/kafka/server/config/Defaults.java:
##
@@ -0,0 +1,278 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.server.config;
+
+import org.apache.kafka.clients.CommonClientConfigs;
+import org.apache.kafka.coordinator.group.Group;
+import org.apache.kafka.coordinator.group.assignor.RangeAssignor;
+import org.apache.kafka.common.config.SaslConfigs;
+import org.apache.kafka.common.config.SslClientAuth;
+import org.apache.kafka.common.config.SslConfigs;
+import org.apache.kafka.common.config.internals.BrokerSecurityConfigs;
+import org.apache.kafka.common.metrics.Sensor;
+import org.apache.kafka.common.network.ListenerName;
+import org.apache.kafka.common.security.auth.KafkaPrincipalBuilder;
+import org.apache.kafka.common.security.auth.SecurityProtocol;
+import
org.apache.kafka.common.security.authenticator.DefaultKafkaPrincipalBuilder;
+import org.apache.kafka.coordinator.group.OffsetConfig;
+import org.apache.kafka.coordinator.transaction.TransactionLogConfig;
+import org.apache.kafka.coordinator.transaction.TransactionStateManagerConfig;
+import org.apache.kafka.raft.RaftConfig;
+import org.apache.kafka.server.common.MetadataVersion;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+
+public class Defaults {
Review Comment:
pr opened here to refactor this #15260
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
OmniaGM commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1464137161
##
server/src/main/java/org/apache/kafka/server/config/Defaults.java:
##
@@ -0,0 +1,278 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.server.config;
+
+import org.apache.kafka.clients.CommonClientConfigs;
+import org.apache.kafka.coordinator.group.Group;
+import org.apache.kafka.coordinator.group.assignor.RangeAssignor;
+import org.apache.kafka.common.config.SaslConfigs;
+import org.apache.kafka.common.config.SslClientAuth;
+import org.apache.kafka.common.config.SslConfigs;
+import org.apache.kafka.common.config.internals.BrokerSecurityConfigs;
+import org.apache.kafka.common.metrics.Sensor;
+import org.apache.kafka.common.network.ListenerName;
+import org.apache.kafka.common.security.auth.KafkaPrincipalBuilder;
+import org.apache.kafka.common.security.auth.SecurityProtocol;
+import
org.apache.kafka.common.security.authenticator.DefaultKafkaPrincipalBuilder;
+import org.apache.kafka.coordinator.group.OffsetConfig;
+import org.apache.kafka.coordinator.transaction.TransactionLogConfig;
+import org.apache.kafka.coordinator.transaction.TransactionStateManagerConfig;
+import org.apache.kafka.raft.RaftConfig;
+import org.apache.kafka.server.common.MetadataVersion;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+
+public class Defaults {
Review Comment:
I think the pattern of default and properties in the class object companion
has been like this in some scala as well. KafkaConfig has been kinda of anti
pattern for a while as the defaults are defined in another object companian. I
will raise another pr soon to move this pr to Java pattern.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
ijuma commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1462200947
##
server/src/main/java/org/apache/kafka/server/config/Defaults.java:
##
@@ -0,0 +1,278 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.server.config;
+
+import org.apache.kafka.clients.CommonClientConfigs;
+import org.apache.kafka.coordinator.group.Group;
+import org.apache.kafka.coordinator.group.assignor.RangeAssignor;
+import org.apache.kafka.common.config.SaslConfigs;
+import org.apache.kafka.common.config.SslClientAuth;
+import org.apache.kafka.common.config.SslConfigs;
+import org.apache.kafka.common.config.internals.BrokerSecurityConfigs;
+import org.apache.kafka.common.metrics.Sensor;
+import org.apache.kafka.common.network.ListenerName;
+import org.apache.kafka.common.security.auth.KafkaPrincipalBuilder;
+import org.apache.kafka.common.security.auth.SecurityProtocol;
+import
org.apache.kafka.common.security.authenticator.DefaultKafkaPrincipalBuilder;
+import org.apache.kafka.coordinator.group.OffsetConfig;
+import org.apache.kafka.coordinator.transaction.TransactionLogConfig;
+import org.apache.kafka.coordinator.transaction.TransactionStateManagerConfig;
+import org.apache.kafka.raft.RaftConfig;
+import org.apache.kafka.server.common.MetadataVersion;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+
+public class Defaults {
Review Comment:
1. If you look at the pattern in Java, the defaults are usually in the same
file as the config file unless it's shared between many configs (then it's
extracted).
2. We can add an exception for checkstyle - it's a tool in 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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
OmniaGM commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1462111203
##
server/src/main/java/org/apache/kafka/server/config/Defaults.java:
##
@@ -0,0 +1,278 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.server.config;
+
+import org.apache.kafka.clients.CommonClientConfigs;
+import org.apache.kafka.coordinator.group.Group;
+import org.apache.kafka.coordinator.group.assignor.RangeAssignor;
+import org.apache.kafka.common.config.SaslConfigs;
+import org.apache.kafka.common.config.SslClientAuth;
+import org.apache.kafka.common.config.SslConfigs;
+import org.apache.kafka.common.config.internals.BrokerSecurityConfigs;
+import org.apache.kafka.common.metrics.Sensor;
+import org.apache.kafka.common.network.ListenerName;
+import org.apache.kafka.common.security.auth.KafkaPrincipalBuilder;
+import org.apache.kafka.common.security.auth.SecurityProtocol;
+import
org.apache.kafka.common.security.authenticator.DefaultKafkaPrincipalBuilder;
+import org.apache.kafka.coordinator.group.OffsetConfig;
+import org.apache.kafka.coordinator.transaction.TransactionLogConfig;
+import org.apache.kafka.coordinator.transaction.TransactionStateManagerConfig;
+import org.apache.kafka.raft.RaftConfig;
+import org.apache.kafka.server.common.MetadataVersion;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+
+public class Defaults {
Review Comment:
1. They always have been separate object out side of KafkaConfig but in the
same file. I just moved them out for reason no. 2
2. Kafkaconfig cant be refactored into Java as one class as it is huge and
breaks the complexity rules for the checkstyle. The plan is to keep them out as
separate class.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
ijuma commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1462039487
##
server/src/main/java/org/apache/kafka/server/config/Defaults.java:
##
@@ -0,0 +1,278 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.server.config;
+
+import org.apache.kafka.clients.CommonClientConfigs;
+import org.apache.kafka.coordinator.group.Group;
+import org.apache.kafka.coordinator.group.assignor.RangeAssignor;
+import org.apache.kafka.common.config.SaslConfigs;
+import org.apache.kafka.common.config.SslClientAuth;
+import org.apache.kafka.common.config.SslConfigs;
+import org.apache.kafka.common.config.internals.BrokerSecurityConfigs;
+import org.apache.kafka.common.metrics.Sensor;
+import org.apache.kafka.common.network.ListenerName;
+import org.apache.kafka.common.security.auth.KafkaPrincipalBuilder;
+import org.apache.kafka.common.security.auth.SecurityProtocol;
+import
org.apache.kafka.common.security.authenticator.DefaultKafkaPrincipalBuilder;
+import org.apache.kafka.coordinator.group.OffsetConfig;
+import org.apache.kafka.coordinator.transaction.TransactionLogConfig;
+import org.apache.kafka.coordinator.transaction.TransactionStateManagerConfig;
+import org.apache.kafka.raft.RaftConfig;
+import org.apache.kafka.server.common.MetadataVersion;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+
+public class Defaults {
Review Comment:
What's the plan for these defaults, they will not be part of `KafkaConfig`
anymore once that's moved to this module?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
ijuma commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1462039487
##
server/src/main/java/org/apache/kafka/server/config/Defaults.java:
##
@@ -0,0 +1,278 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.server.config;
+
+import org.apache.kafka.clients.CommonClientConfigs;
+import org.apache.kafka.coordinator.group.Group;
+import org.apache.kafka.coordinator.group.assignor.RangeAssignor;
+import org.apache.kafka.common.config.SaslConfigs;
+import org.apache.kafka.common.config.SslClientAuth;
+import org.apache.kafka.common.config.SslConfigs;
+import org.apache.kafka.common.config.internals.BrokerSecurityConfigs;
+import org.apache.kafka.common.metrics.Sensor;
+import org.apache.kafka.common.network.ListenerName;
+import org.apache.kafka.common.security.auth.KafkaPrincipalBuilder;
+import org.apache.kafka.common.security.auth.SecurityProtocol;
+import
org.apache.kafka.common.security.authenticator.DefaultKafkaPrincipalBuilder;
+import org.apache.kafka.coordinator.group.OffsetConfig;
+import org.apache.kafka.coordinator.transaction.TransactionLogConfig;
+import org.apache.kafka.coordinator.transaction.TransactionStateManagerConfig;
+import org.apache.kafka.raft.RaftConfig;
+import org.apache.kafka.server.common.MetadataVersion;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+
+public class Defaults {
Review Comment:
What's the plan for these defaults, they will not be part of `KafkaConfig`
anymore?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
mimaison merged PR #15158: URL: https://github.com/apache/kafka/pull/15158 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
mimaison commented on code in PR #15158: URL: https://github.com/apache/kafka/pull/15158#discussion_r1459178820 ## core/src/test/scala/unit/kafka/utils/PasswordEncoderTest.scala: ## @@ -19,10 +19,9 @@ package kafka.utils import javax.crypto.SecretKeyFactory - -import kafka.server.Defaults import org.apache.kafka.common.config.ConfigException import org.apache.kafka.common.config.types.Password +import org.apache.kafka.server.config.Defaults Review Comment: Ah yes, my bad! -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
OmniaGM commented on code in PR #15158: URL: https://github.com/apache/kafka/pull/15158#discussion_r1459169205 ## checkstyle/import-control-core.xml: ## @@ -82,6 +82,7 @@ + Review Comment: nope, I think this was left over from the main pr -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
OmniaGM commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1459168180
##
core/src/test/scala/unit/kafka/log/LogCleanerManagerTest.scala:
##
@@ -53,7 +53,7 @@ class LogCleanerManagerTest extends Logging {
val logConfig: LogConfig = new LogConfig(logProps)
val time = new MockTime(14000L, 1000L) // Tue May 13 16:53:20 UTC
2014 for `currentTimeMs`
val offset = 999
- val producerStateManagerConfig = new
ProducerStateManagerConfig(kafka.server.Defaults.ProducerIdExpirationMs, false)
+ val producerStateManagerConfig = new
ProducerStateManagerConfig(org.apache.kafka.server.config.Defaults.PRODUCER_ID_EXPIRATION_MS,
false)
Review Comment:
done
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
OmniaGM commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1459167766
##
server/src/main/java/org/apache/kafka/server/config/Defaults.java:
##
@@ -0,0 +1,278 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.server.config;
+
+import org.apache.kafka.clients.CommonClientConfigs;
+import org.apache.kafka.coordinator.group.Group;
+import org.apache.kafka.coordinator.group.assignor.RangeAssignor;
+import org.apache.kafka.common.config.SaslConfigs;
+import org.apache.kafka.common.config.SslClientAuth;
+import org.apache.kafka.common.config.SslConfigs;
+import org.apache.kafka.common.config.internals.BrokerSecurityConfigs;
+import org.apache.kafka.common.metrics.Sensor;
+import org.apache.kafka.common.network.ListenerName;
+import org.apache.kafka.common.security.auth.KafkaPrincipalBuilder;
+import org.apache.kafka.common.security.auth.SecurityProtocol;
+import
org.apache.kafka.common.security.authenticator.DefaultKafkaPrincipalBuilder;
+import org.apache.kafka.coordinator.group.OffsetConfig;
+import org.apache.kafka.coordinator.transaction.TransactionLogConfig;
+import org.apache.kafka.coordinator.transaction.TransactionStateManagerConfig;
+import org.apache.kafka.raft.RaftConfig;
+import org.apache.kafka.server.common.MetadataVersion;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+
+public class Defaults {
+/** * Zookeeper Configuration */
+public static final int ZK_SESSION_TIMEOUT_MS = 18000;
+public static final boolean ZK_ENABLE_SECURE_ACLS = false;
+public static final int ZK_MAX_IN_FLIGHT_REQUESTS = 10;
+public static final boolean ZK_SSL_CLIENT_ENABLE = false;
+public static final String ZK_SSL_PROTOCOL = "TLSv1.2";
+public static final String ZK_SSL_ENDPOINT_IDENTIFICATION_ALGORITHM =
"HTTPS";
+public static final boolean ZK_SSL_CRL_ENABLE = false;
+public static final boolean ZK_SSL_OCSP_ENABLE = false;
+
+/** * General Configuration */
+public static final boolean BROKER_ID_GENERATION_ENABLE = true;
+public static final int MAX_RESERVED_BROKER_ID = 1000;
+public static final int BROKER_ID = -1;
+public static final int NUM_NETWORK_THREADS = 3;
+public static final int NUM_IO_THREADS = 8;
+public static final int BACKGROUND_THREADS = 10;
+public static final int QUEUED_MAX_REQUESTS = 500;
+public static final int QUEUED_MAX_REQUEST_BYTES = -1;
+public static final int INITIAL_BROKER_REGISTRATION_TIMEOUT_MS = 6;
+public static final int BROKER_HEARTBEAT_INTERVAL_MS = 2000;
+public static final int BROKER_SESSION_TIMEOUT_MS = 9000;
+public static final int METADATA_SNAPSHOT_MAX_NEW_RECORD_BYTES = 20 * 1024
* 1024;
+public static final long METADATA_SNAPSHOT_MAX_INTERVAL_MS =
TimeUnit.HOURS.toMillis(1);
+public static final int METADATA_MAX_IDLE_INTERVAL_MS = 500;
+public static final int METADATA_MAX_RETENTION_BYTES = 100 * 1024 * 1024;
+public static final boolean DELETE_TOPIC_ENABLE = true;
+/** * KRaft mode configs */
+public static final int EMPTY_NODE_ID = -1;
+public static final long SERVER_MAX_STARTUP_TIME_MS = Long.MAX_VALUE;
+
+/* Authorizer Configuration ***/
+public static final String AUTHORIZER_CLASS_NAME = "";
+
+/** * Socket Server Configuration */
+public static final String LISTENERS = "PLAINTEXT://:9092";
+//TODO: Replace this with EndPoint.DefaultSecurityProtocolMap once
EndPoint is out of core.
+public static final String LISTENER_SECURITY_PROTOCOL_MAP =
Arrays.stream(SecurityProtocol.values())
+.collect(Collectors.toMap(sp ->
ListenerName.forSecurityProtocol(sp), sp -> sp))
+.entrySet()
+.stream()
+.map(entry -> entry.getKey().value() + ":" +
entry.getValue().name())
+.collect(Collectors.joining(","));
+public static final int SOCKET_SEND_BUFFER_BYTES = 100 * 1024;
+public static final int SOCKET_RECEIVE_BUFF
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
OmniaGM commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1459167324
##
server/src/main/java/org/apache/kafka/server/config/Defaults.java:
##
@@ -0,0 +1,278 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.server.config;
+
+import org.apache.kafka.clients.CommonClientConfigs;
+import org.apache.kafka.coordinator.group.Group;
+import org.apache.kafka.coordinator.group.assignor.RangeAssignor;
+import org.apache.kafka.common.config.SaslConfigs;
+import org.apache.kafka.common.config.SslClientAuth;
+import org.apache.kafka.common.config.SslConfigs;
+import org.apache.kafka.common.config.internals.BrokerSecurityConfigs;
+import org.apache.kafka.common.metrics.Sensor;
+import org.apache.kafka.common.network.ListenerName;
+import org.apache.kafka.common.security.auth.KafkaPrincipalBuilder;
+import org.apache.kafka.common.security.auth.SecurityProtocol;
+import
org.apache.kafka.common.security.authenticator.DefaultKafkaPrincipalBuilder;
+import org.apache.kafka.coordinator.group.OffsetConfig;
+import org.apache.kafka.coordinator.transaction.TransactionLogConfig;
+import org.apache.kafka.coordinator.transaction.TransactionStateManagerConfig;
+import org.apache.kafka.raft.RaftConfig;
+import org.apache.kafka.server.common.MetadataVersion;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+
+public class Defaults {
+/** * Zookeeper Configuration */
+public static final int ZK_SESSION_TIMEOUT_MS = 18000;
+public static final boolean ZK_ENABLE_SECURE_ACLS = false;
+public static final int ZK_MAX_IN_FLIGHT_REQUESTS = 10;
+public static final boolean ZK_SSL_CLIENT_ENABLE = false;
+public static final String ZK_SSL_PROTOCOL = "TLSv1.2";
+public static final String ZK_SSL_ENDPOINT_IDENTIFICATION_ALGORITHM =
"HTTPS";
+public static final boolean ZK_SSL_CRL_ENABLE = false;
+public static final boolean ZK_SSL_OCSP_ENABLE = false;
+
+/** * General Configuration */
+public static final boolean BROKER_ID_GENERATION_ENABLE = true;
+public static final int MAX_RESERVED_BROKER_ID = 1000;
+public static final int BROKER_ID = -1;
+public static final int NUM_NETWORK_THREADS = 3;
+public static final int NUM_IO_THREADS = 8;
+public static final int BACKGROUND_THREADS = 10;
+public static final int QUEUED_MAX_REQUESTS = 500;
+public static final int QUEUED_MAX_REQUEST_BYTES = -1;
+public static final int INITIAL_BROKER_REGISTRATION_TIMEOUT_MS = 6;
+public static final int BROKER_HEARTBEAT_INTERVAL_MS = 2000;
+public static final int BROKER_SESSION_TIMEOUT_MS = 9000;
+public static final int METADATA_SNAPSHOT_MAX_NEW_RECORD_BYTES = 20 * 1024
* 1024;
+public static final long METADATA_SNAPSHOT_MAX_INTERVAL_MS =
TimeUnit.HOURS.toMillis(1);
+public static final int METADATA_MAX_IDLE_INTERVAL_MS = 500;
+public static final int METADATA_MAX_RETENTION_BYTES = 100 * 1024 * 1024;
+public static final boolean DELETE_TOPIC_ENABLE = true;
+/** * KRaft mode configs */
+public static final int EMPTY_NODE_ID = -1;
+public static final long SERVER_MAX_STARTUP_TIME_MS = Long.MAX_VALUE;
+
+/* Authorizer Configuration ***/
+public static final String AUTHORIZER_CLASS_NAME = "";
+
+/** * Socket Server Configuration */
+public static final String LISTENERS = "PLAINTEXT://:9092";
+//TODO: Replace this with EndPoint.DefaultSecurityProtocolMap once
EndPoint is out of core.
+public static final String LISTENER_SECURITY_PROTOCOL_MAP =
Arrays.stream(SecurityProtocol.values())
+.collect(Collectors.toMap(sp ->
ListenerName.forSecurityProtocol(sp), sp -> sp))
+.entrySet()
+.stream()
+.map(entry -> entry.getKey().value() + ":" +
entry.getValue().name())
+.collect(Collectors.joining(","));
+public static final int SOCKET_SEND_BUFFER_BYTES = 100 * 1024;
+public static final int SOCKET_RECEIVE_BUFF
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
OmniaGM commented on code in PR #15158: URL: https://github.com/apache/kafka/pull/15158#discussion_r1459158610 ## core/src/test/scala/unit/kafka/utils/PasswordEncoderTest.scala: ## @@ -19,10 +19,9 @@ package kafka.utils import javax.crypto.SecretKeyFactory - -import kafka.server.Defaults import org.apache.kafka.common.config.ConfigException import org.apache.kafka.common.config.types.Password +import org.apache.kafka.server.config.Defaults Review Comment: The other 2 lines are imports of `org.apache.kafka.common` and not `org.apache.kafka.server` -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
mimaison commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1458684850
##
checkstyle/import-control-core.xml:
##
@@ -82,6 +82,7 @@
+
Review Comment:
Do we really need this change? core seems to build fine without it.
##
core/src/test/scala/other/kafka/TestLinearWriteSpeed.scala:
##
@@ -220,8 +220,8 @@ object TestLinearWriteSpeed {
brokerTopicStats = new BrokerTopicStats,
time = Time.SYSTEM,
maxTransactionTimeoutMs = 5 * 60 * 1000,
- producerStateManagerConfig = new
ProducerStateManagerConfig(kafka.server.Defaults.ProducerIdExpirationMs, false),
- producerIdExpirationCheckIntervalMs =
kafka.server.Defaults.ProducerIdExpirationCheckIntervalMs,
+ producerStateManagerConfig = new
ProducerStateManagerConfig(org.apache.kafka.server.config.Defaults.PRODUCER_ID_EXPIRATION_MS,
false),
Review Comment:
Can we import `org.apache.kafka.server.config.Defaults`?
##
core/src/test/scala/unit/kafka/log/LogCleanerManagerTest.scala:
##
@@ -53,7 +53,7 @@ class LogCleanerManagerTest extends Logging {
val logConfig: LogConfig = new LogConfig(logProps)
val time = new MockTime(14000L, 1000L) // Tue May 13 16:53:20 UTC
2014 for `currentTimeMs`
val offset = 999
- val producerStateManagerConfig = new
ProducerStateManagerConfig(kafka.server.Defaults.ProducerIdExpirationMs, false)
+ val producerStateManagerConfig = new
ProducerStateManagerConfig(org.apache.kafka.server.config.Defaults.PRODUCER_ID_EXPIRATION_MS,
false)
Review Comment:
Can we import `org.apache.kafka.server.config.Defaults`?
##
core/src/main/scala/kafka/server/KafkaConfig.scala:
##
@@ -1336,205 +1097,205 @@ object KafkaConfig {
.define(LogMessageDownConversionEnableProp, BOOLEAN,
LogConfig.DEFAULT_MESSAGE_DOWNCONVERSION_ENABLE, LOW,
LogMessageDownConversionEnableDoc)
/** * Replication configuration ***/
- .define(ControllerSocketTimeoutMsProp, INT,
Defaults.ControllerSocketTimeoutMs, MEDIUM, ControllerSocketTimeoutMsDoc)
- .define(DefaultReplicationFactorProp, INT,
Defaults.DefaultReplicationFactor, MEDIUM, DefaultReplicationFactorDoc)
- .define(ReplicaLagTimeMaxMsProp, LONG, Defaults.ReplicaLagTimeMaxMs,
HIGH, ReplicaLagTimeMaxMsDoc)
- .define(ReplicaSocketTimeoutMsProp, INT,
Defaults.ReplicaSocketTimeoutMs, HIGH, ReplicaSocketTimeoutMsDoc)
- .define(ReplicaSocketReceiveBufferBytesProp, INT,
Defaults.ReplicaSocketReceiveBufferBytes, HIGH,
ReplicaSocketReceiveBufferBytesDoc)
- .define(ReplicaFetchMaxBytesProp, INT, Defaults.ReplicaFetchMaxBytes,
atLeast(0), MEDIUM, ReplicaFetchMaxBytesDoc)
- .define(ReplicaFetchWaitMaxMsProp, INT, Defaults.ReplicaFetchWaitMaxMs,
HIGH, ReplicaFetchWaitMaxMsDoc)
- .define(ReplicaFetchBackoffMsProp, INT, Defaults.ReplicaFetchBackoffMs,
atLeast(0), MEDIUM, ReplicaFetchBackoffMsDoc)
- .define(ReplicaFetchMinBytesProp, INT, Defaults.ReplicaFetchMinBytes,
HIGH, ReplicaFetchMinBytesDoc)
- .define(ReplicaFetchResponseMaxBytesProp, INT,
Defaults.ReplicaFetchResponseMaxBytes, atLeast(0), MEDIUM,
ReplicaFetchResponseMaxBytesDoc)
- .define(NumReplicaFetchersProp, INT, Defaults.NumReplicaFetchers, HIGH,
NumReplicaFetchersDoc)
- .define(ReplicaHighWatermarkCheckpointIntervalMsProp, LONG,
Defaults.ReplicaHighWatermarkCheckpointIntervalMs, HIGH,
ReplicaHighWatermarkCheckpointIntervalMsDoc)
- .define(FetchPurgatoryPurgeIntervalRequestsProp, INT,
Defaults.FetchPurgatoryPurgeIntervalRequests, MEDIUM,
FetchPurgatoryPurgeIntervalRequestsDoc)
- .define(ProducerPurgatoryPurgeIntervalRequestsProp, INT,
Defaults.ProducerPurgatoryPurgeIntervalRequests, MEDIUM,
ProducerPurgatoryPurgeIntervalRequestsDoc)
- .define(DeleteRecordsPurgatoryPurgeIntervalRequestsProp, INT,
Defaults.DeleteRecordsPurgatoryPurgeIntervalRequests, MEDIUM,
DeleteRecordsPurgatoryPurgeIntervalRequestsDoc)
- .define(AutoLeaderRebalanceEnableProp, BOOLEAN,
Defaults.AutoLeaderRebalanceEnable, HIGH, AutoLeaderRebalanceEnableDoc)
- .define(LeaderImbalancePerBrokerPercentageProp, INT,
Defaults.LeaderImbalancePerBrokerPercentage, HIGH,
LeaderImbalancePerBrokerPercentageDoc)
- .define(LeaderImbalanceCheckIntervalSecondsProp, LONG,
Defaults.LeaderImbalanceCheckIntervalSeconds, atLeast(1), HIGH,
LeaderImbalanceCheckIntervalSecondsDoc)
+ .define(ControllerSocketTimeoutMsProp, INT,
Defaults.CONTROLLER_SOCKET_TIMEOUT_MS, MEDIUM, ControllerSocketTimeoutMsDoc)
+ .define(DefaultReplicationFactorProp, INT, Defaults.REPLICATION_FACTOR,
MEDIUM, DefaultReplicationFactorDoc)
+ .define(ReplicaLagTimeMaxMsProp, LONG, Defaults.REPLICA_LAG_TIME_MAX_MS,
HIGH, ReplicaLagTimeMaxMsDoc)
+ .define(ReplicaSocketTimeoutMsProp, INT,
Defaults.REPLICA_SOCKET_TIMEOUT_MS, HIGH, ReplicaSocketTimeoutMsDoc)
+ .define(ReplicaSocketReceiveBufferBytesP
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
OmniaGM commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1457856799
##
transaction-coordinator/src/main/java/org/apache/kafka/coordinator/transaction/TransactionLogConfig.java:
##
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.coordinator.transaction;
+
+public class TransactionLogConfig {
Review Comment:
it will be very small one which I don't believe it worth it.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
nizhikov commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1457709089
##
transaction-coordinator/src/main/java/org/apache/kafka/coordinator/transaction/TransactionLogConfig.java:
##
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.coordinator.transaction;
+
+public class TransactionLogConfig {
Review Comment:
Can ew introduce new module and config into separate PR?
##
transaction-coordinator/src/main/java/org/apache/kafka/coordinator/transaction/TransactionLogConfig.java:
##
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.coordinator.transaction;
+
+public class TransactionLogConfig {
Review Comment:
Can we introduce new module and config into separate PR?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
nizhikov commented on code in PR #15158: URL: https://github.com/apache/kafka/pull/15158#discussion_r1457704867 ## checkstyle/import-control.xml: ## @@ -261,6 +261,10 @@ + + Review Comment: Do we really need this empty block? -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
nizhikov commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1457704365
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/OffsetConfig.java:
##
@@ -20,6 +20,7 @@
import org.apache.kafka.common.record.CompressionType;
public class OffsetConfig {
+
Review Comment:
Do we really need this empty line?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
OmniaGM commented on PR #15158: URL: https://github.com/apache/kafka/pull/15158#issuecomment-1894343924 > @OmniaGM It looks like there are a [few build failures](https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-15158/6/pipeline/10): > > ``` > [Error] /home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-15158/core/src/main/scala/kafka/server/DynamicConfig.scala:26:58: Unused import > [Error] /home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-15158/core/src/main/scala/kafka/server/KafkaConfig.scala:47:50: Unused import > ``` fixed this -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
OmniaGM commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1453877298
##
transaction-coordinator/src/main/java/org/apache/kafka/coordinator/transaction/TransactionLogConfig.java:
##
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.coordinator.transaction;
+
+public class TransactionLogConfig {
+// Log-level config default values
+public static final int DEFAULT_NUM_PARTITIONS = 50;
+public static final int DEFAULT_SEGMENT_BYTES = 100 * 1024 * 1024;
+public static final short DEFAULT_REPLICATION_FACTOR = 3;
+public static final int DEFAULT_MIN_IN_SYNC_REPLICAS = 2;
+public static final int DEFAULT_LOAD_BUFFER_SIZE = 5 * 1024 * 1024;
+}
Review Comment:
Done, and did the same to other files I created as well
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
mimaison commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1453175220
##
transaction-coordinator/src/main/java/org/apache/kafka/coordinator/transaction/TransactionLogConfig.java:
##
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.coordinator.transaction;
+
+public class TransactionLogConfig {
+// Log-level config default values
+public static final int DEFAULT_NUM_PARTITIONS = 50;
+public static final int DEFAULT_SEGMENT_BYTES = 100 * 1024 * 1024;
+public static final short DEFAULT_REPLICATION_FACTOR = 3;
+public static final int DEFAULT_MIN_IN_SYNC_REPLICAS = 2;
+public static final int DEFAULT_LOAD_BUFFER_SIZE = 5 * 1024 * 1024;
+}
Review Comment:
Nit: We usually have a trailing line at the end of files.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
mimaison commented on PR #15158: URL: https://github.com/apache/kafka/pull/15158#issuecomment-1893389463 @OmniaGM It looks like there are a [few build failures](https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-15158/6/pipeline/10): ``` [Error] /home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-15158/core/src/main/scala/kafka/server/DynamicConfig.scala:26:58: Unused import [Error] /home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-15158/core/src/main/scala/kafka/server/KafkaConfig.scala:47:50: Unused import ``` -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
OmniaGM commented on PR #15158: URL: https://github.com/apache/kafka/pull/15158#issuecomment-1892798596 > @OmniaGM Can we rebase to resolve the conflicts? Thanks done now. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
mimaison commented on PR #15158: URL: https://github.com/apache/kafka/pull/15158#issuecomment-1888774059 @OmniaGM Can we rebase to resolve the conflicts? Thanks -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
OmniaGM commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1449013239
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/transaction/TransactionLogConfig.java:
##
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.coordinator.transaction;
+
+public class TransactionLogConfig {
Review Comment:
I added `transaction-coordinator` modules and moved theses there
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
ijuma commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1447757987
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/transaction/TransactionLogConfig.java:
##
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.coordinator.transaction;
+
+public class TransactionLogConfig {
Review Comment:
Yeah, I was using the maven name. I agree that we shouldn't add a dependency
unless it exists.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
dajac commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1447491926
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/transaction/TransactionLogConfig.java:
##
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.coordinator.transaction;
+
+public class TransactionLogConfig {
Review Comment:
Yeah, creating a new module makes sense to me. I would call it
`transaction-coordinator` and the jar would be called
`kafka-transaction-coordinator` in order to follow the naming scheme of the
`group-coordinator`. I am not sure if it really needs to depend on the
group-coordinator module though but I haven't not checked the details.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
ijuma commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1447483283
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/transaction/TransactionLogConfig.java:
##
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.coordinator.transaction;
+
+public class TransactionLogConfig {
Review Comment:
Let's see what @dajac says, but I think a `kafka-transactions` or
`kafka-transactions-coordinator` module that depends on
`kafka-group-coordinator` makes sense to me. The former option is shorter and
as clear while the second follows the same pattern as for the group coordinator
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
OmniaGM commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1446673095
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/transaction/TransactionLogConfig.java:
##
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.coordinator.transaction;
+
+public class TransactionLogConfig {
Review Comment:
I feel the same but also not sure where they should move. They don't fit in
server module either. I don't see any Jiras to move transaction coordinator out
of server but maybe I can start a new module for transaction coordinator
similar to the group one. Would this make more sense?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
dajac commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1446669238
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/transaction/TransactionLogConfig.java:
##
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.coordinator.transaction;
+
+public class TransactionLogConfig {
Review Comment:
Hey @OmniaGM. It is a bit weird to have those transaction classes in the
group-coordinator module. It does not seem to be the correct place.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
OmniaGM commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1446313774
##
core/src/main/scala/kafka/coordinator/group/GroupCoordinator.scala:
##
@@ -1751,17 +1752,17 @@ object GroupCoordinator {
GroupCoordinator(config, replicaManager, heartbeatPurgatory,
rebalancePurgatory, time, metrics)
}
- private[group] def offsetConfig(config: KafkaConfig) = OffsetConfig(
-maxMetadataSize = config.offsetMetadataMaxSize,
-loadBufferSize = config.offsetsLoadBufferSize,
-offsetsRetentionMs = config.offsetsRetentionMinutes * 60L * 1000L,
-offsetsRetentionCheckIntervalMs = config.offsetsRetentionCheckIntervalMs,
-offsetsTopicNumPartitions = config.offsetsTopicPartitions,
-offsetsTopicSegmentBytes = config.offsetsTopicSegmentBytes,
-offsetsTopicReplicationFactor = config.offsetsTopicReplicationFactor,
-offsetsTopicCompressionType = config.offsetsTopicCompressionType,
-offsetCommitTimeoutMs = config.offsetCommitTimeoutMs,
-offsetCommitRequiredAcks = config.offsetCommitRequiredAcks
+ private[group] def offsetConfig(config: KafkaConfig) = new OffsetConfig(
Review Comment:
#15161
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
OmniaGM commented on code in PR #15158: URL: https://github.com/apache/kafka/pull/15158#discussion_r1446295496 ## core/src/main/scala/kafka/server/ReplicationQuotaManager.scala: ## @@ -39,18 +39,9 @@ import org.apache.kafka.common.utils.Time * @param quotaWindowSizeSeconds The time span of each sample * */ -case class ReplicationQuotaManagerConfig(quotaBytesPerSecondDefault: Long = QuotaBytesPerSecondDefault, Review Comment: done here #15160 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
OmniaGM commented on code in PR #15158: URL: https://github.com/apache/kafka/pull/15158#discussion_r1446294876 ## core/src/main/scala/kafka/server/ClientQuotaManager.scala: ## @@ -51,15 +52,9 @@ case class ClientSensors(metricTags: Map[String, String], quotaSensor: Sensor, t * */ case class ClientQuotaManagerConfig(numQuotaSamples: Int = Review Comment: #15159 I moved this to another pr -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
nizhikov commented on code in PR #15158: URL: https://github.com/apache/kafka/pull/15158#discussion_r1446264476 ## core/src/main/scala/kafka/server/ReplicationQuotaManager.scala: ## @@ -39,18 +39,9 @@ import org.apache.kafka.common.utils.Time * @param quotaWindowSizeSeconds The time span of each sample * */ -case class ReplicationQuotaManagerConfig(quotaBytesPerSecondDefault: Long = QuotaBytesPerSecondDefault, Review Comment: Don't hesitate to ping me for review. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
OmniaGM commented on code in PR #15158: URL: https://github.com/apache/kafka/pull/15158#discussion_r1446247860 ## core/src/main/scala/kafka/server/ReplicationQuotaManager.scala: ## @@ -39,18 +39,9 @@ import org.apache.kafka.common.utils.Time * @param quotaWindowSizeSeconds The time span of each sample * */ -case class ReplicationQuotaManagerConfig(quotaBytesPerSecondDefault: Long = QuotaBytesPerSecondDefault, Review Comment: On a second thought let's break them into another pr because I'll need them for KafkaConfig anyway later -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
nizhikov commented on code in PR #15158: URL: https://github.com/apache/kafka/pull/15158#discussion_r1446246355 ## core/src/main/scala/kafka/server/ReplicationQuotaManager.scala: ## @@ -39,18 +39,9 @@ import org.apache.kafka.common.utils.Time * @param quotaWindowSizeSeconds The time span of each sample * */ -case class ReplicationQuotaManagerConfig(quotaBytesPerSecondDefault: Long = QuotaBytesPerSecondDefault, Review Comment: > If this will make it easier It will make changes easier in my opinion. Moving things atomically with clear changes is always better choice. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
OmniaGM commented on code in PR #15158: URL: https://github.com/apache/kafka/pull/15158#discussion_r1446242987 ## core/src/main/scala/kafka/server/ClientQuotaManager.scala: ## @@ -51,15 +52,9 @@ case class ClientSensors(metricTags: Map[String, String], quotaSensor: Sensor, t * */ case class ClientQuotaManagerConfig(numQuotaSamples: Int = Review Comment: > Do we really need instance fields in java version in this case? Maybe it will be more clear to keep only constants. remove the contractor make sense for now -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
OmniaGM commented on code in PR #15158: URL: https://github.com/apache/kafka/pull/15158#discussion_r1446241768 ## core/src/main/scala/kafka/server/ReplicationQuotaManager.scala: ## @@ -39,18 +39,9 @@ import org.apache.kafka.common.utils.Time * @param quotaWindowSizeSeconds The time span of each sample * */ -case class ReplicationQuotaManagerConfig(quotaBytesPerSecondDefault: Long = QuotaBytesPerSecondDefault, Review Comment: I can move them to another pr before to be merged before this one. If this will make it easier -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
nizhikov commented on code in PR #15158: URL: https://github.com/apache/kafka/pull/15158#discussion_r1446240650 ## core/src/main/scala/kafka/server/ClientQuotaManager.scala: ## @@ -51,15 +52,9 @@ case class ClientSensors(metricTags: Map[String, String], quotaSensor: Sensor, t * */ case class ClientQuotaManagerConfig(numQuotaSamples: Int = Review Comment: Do we really need instance fields in java version in this case? Maybe it will be more clear to keep only constants. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
OmniaGM commented on code in PR #15158: URL: https://github.com/apache/kafka/pull/15158#discussion_r1446237069 ## core/src/main/scala/kafka/server/ClientQuotaManager.scala: ## @@ -51,15 +52,9 @@ case class ClientSensors(metricTags: Map[String, String], quotaSensor: Sensor, t * */ case class ClientQuotaManagerConfig(numQuotaSamples: Int = Review Comment: In the JIRA we agreed on only moving the object companions ## core/src/main/scala/kafka/server/ReplicationQuotaManager.scala: ## @@ -39,18 +39,9 @@ import org.apache.kafka.common.utils.Time * @param quotaWindowSizeSeconds The time span of each sample * */ -case class ReplicationQuotaManagerConfig(quotaBytesPerSecondDefault: Long = QuotaBytesPerSecondDefault, Review Comment: Same as above -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
OmniaGM commented on code in PR #15158: URL: https://github.com/apache/kafka/pull/15158#discussion_r1446236478 ## core/src/main/scala/kafka/server/ClientQuotaManager.scala: ## @@ -51,15 +52,9 @@ case class ClientSensors(metricTags: Map[String, String], quotaSensor: Sensor, t * */ case class ClientQuotaManagerConfig(numQuotaSamples: Int = Review Comment: Defaults depends only on `ClientQuotaManagerConfig` object companion only. Moving `ClientQuotaManagerConfig` class can happened when we move `ClientQuotaManager` or in another pr as this will impact QuotaFactory and some Quota test classes -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
nizhikov commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1446236035
##
core/src/main/scala/kafka/coordinator/group/GroupCoordinator.scala:
##
@@ -1751,17 +1752,17 @@ object GroupCoordinator {
GroupCoordinator(config, replicaManager, heartbeatPurgatory,
rebalancePurgatory, time, metrics)
}
- private[group] def offsetConfig(config: KafkaConfig) = OffsetConfig(
-maxMetadataSize = config.offsetMetadataMaxSize,
-loadBufferSize = config.offsetsLoadBufferSize,
-offsetsRetentionMs = config.offsetsRetentionMinutes * 60L * 1000L,
-offsetsRetentionCheckIntervalMs = config.offsetsRetentionCheckIntervalMs,
-offsetsTopicNumPartitions = config.offsetsTopicPartitions,
-offsetsTopicSegmentBytes = config.offsetsTopicSegmentBytes,
-offsetsTopicReplicationFactor = config.offsetsTopicReplicationFactor,
-offsetsTopicCompressionType = config.offsetsTopicCompressionType,
-offsetCommitTimeoutMs = config.offsetCommitTimeoutMs,
-offsetCommitRequiredAcks = config.offsetCommitRequiredAcks
+ private[group] def offsetConfig(config: KafkaConfig) = new OffsetConfig(
Review Comment:
Thanks. Now I see it.
I prefer to have things as atomic as possible so I prefer to move
`OffsetConfig` and other `Defaults` dependencies as separate changes prior to
this PR. But it's up to you to decide.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
OmniaGM commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1446231096
##
core/src/main/scala/kafka/coordinator/group/GroupCoordinator.scala:
##
@@ -1751,17 +1752,17 @@ object GroupCoordinator {
GroupCoordinator(config, replicaManager, heartbeatPurgatory,
rebalancePurgatory, time, metrics)
}
- private[group] def offsetConfig(config: KafkaConfig) = OffsetConfig(
-maxMetadataSize = config.offsetMetadataMaxSize,
-loadBufferSize = config.offsetsLoadBufferSize,
-offsetsRetentionMs = config.offsetsRetentionMinutes * 60L * 1000L,
-offsetsRetentionCheckIntervalMs = config.offsetsRetentionCheckIntervalMs,
-offsetsTopicNumPartitions = config.offsetsTopicPartitions,
-offsetsTopicSegmentBytes = config.offsetsTopicSegmentBytes,
-offsetsTopicReplicationFactor = config.offsetsTopicReplicationFactor,
-offsetsTopicCompressionType = config.offsetsTopicCompressionType,
-offsetCommitTimeoutMs = config.offsetCommitTimeoutMs,
-offsetCommitRequiredAcks = config.offsetCommitRequiredAcks
+ private[group] def offsetConfig(config: KafkaConfig) = new OffsetConfig(
Review Comment:
Defaults depends on `OffsetConfig` object companion which if we remove it
from the `OffsetConfig` file will one have `OffsetConfig` class signature. I
vote to keep it here as it is very small 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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
nizhikov commented on code in PR #15158: URL: https://github.com/apache/kafka/pull/15158#discussion_r1446229388 ## core/src/main/scala/kafka/server/ReplicationQuotaManager.scala: ## @@ -39,18 +39,9 @@ import org.apache.kafka.common.utils.Time * @param quotaWindowSizeSeconds The time span of each sample * */ -case class ReplicationQuotaManagerConfig(quotaBytesPerSecondDefault: Long = QuotaBytesPerSecondDefault, Review Comment: We have new `ReplicationQuotaManagerConfig.java`. In the same time scala version of class is here, also. It seems like we need to keep only one of them. I think changes related to `ReplicationQuotaManagerConfig` can be moved to another PR to simplify review and reduce 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
nizhikov commented on code in PR #15158: URL: https://github.com/apache/kafka/pull/15158#discussion_r1446225594 ## core/src/main/scala/kafka/server/ClientQuotaManager.scala: ## @@ -51,15 +52,9 @@ case class ClientSensors(metricTags: Map[String, String], quotaSensor: Sensor, t * */ case class ClientQuotaManagerConfig(numQuotaSamples: Int = Review Comment: We have new `ClientQuotaManagerConfig.java`. In the same time scala version of class is here, also. It seems like we need to keep only one of them. I think changes related to `ClientQuotaManagerConfig` can be moved to another PR to simplify review and reduce 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
nizhikov commented on code in PR #15158:
URL: https://github.com/apache/kafka/pull/15158#discussion_r1446222307
##
core/src/main/scala/kafka/coordinator/group/GroupCoordinator.scala:
##
@@ -1751,17 +1752,17 @@ object GroupCoordinator {
GroupCoordinator(config, replicaManager, heartbeatPurgatory,
rebalancePurgatory, time, metrics)
}
- private[group] def offsetConfig(config: KafkaConfig) = OffsetConfig(
-maxMetadataSize = config.offsetMetadataMaxSize,
-loadBufferSize = config.offsetsLoadBufferSize,
-offsetsRetentionMs = config.offsetsRetentionMinutes * 60L * 1000L,
-offsetsRetentionCheckIntervalMs = config.offsetsRetentionCheckIntervalMs,
-offsetsTopicNumPartitions = config.offsetsTopicPartitions,
-offsetsTopicSegmentBytes = config.offsetsTopicSegmentBytes,
-offsetsTopicReplicationFactor = config.offsetsTopicReplicationFactor,
-offsetsTopicCompressionType = config.offsetsTopicCompressionType,
-offsetCommitTimeoutMs = config.offsetCommitTimeoutMs,
-offsetCommitRequiredAcks = config.offsetCommitRequiredAcks
+ private[group] def offsetConfig(config: KafkaConfig) = new OffsetConfig(
Review Comment:
`OffsetConfig` seems not related to `Defaults`.
Shoule we move it to another PR?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15853: Move KafkaConfig.Defaults to server module [kafka]
OmniaGM commented on PR #15158: URL: https://github.com/apache/kafka/pull/15158#issuecomment-1883206041 @mimaison @ijuma can you have a look please? -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
