KAFKA-15853: Move KafkaConfig.Defaults to server module #15158

OmniaGM · 2024-01-09T14:37:22Z

first pr to break #15103

Moving any object companion that KafkaConfigs.Defaults depends on outside of core.
- ClientQuotaManagerConfig, and ReplicationQuotaManagerConfig object companions moved to org.apache.kafka.server.config in another pr need to be merged first
- TransactionLogConfig object companion moved to org.apache.kafka.coordinator.transaction
- OffsetConfig moved to org.apache.kafka.coordinator.group

Committer Checklist (excluded from commit message)

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

OmniaGM · 2024-01-09T14:57:47Z

@mimaison @ijuma can you have a look please?

core/src/main/scala/kafka/coordinator/group/GroupCoordinator.scala

core/src/main/scala/kafka/server/ClientQuotaManager.scala

core/src/main/scala/kafka/server/ReplicationQuotaManager.scala

dajac · 2024-01-09T22:06:05Z

...coordinator/src/main/java/org/apache/kafka/coordinator/transaction/TransactionLogConfig.java

+ */
+package org.apache.kafka.coordinator.transaction;
+
+public class TransactionLogConfig {


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.

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?

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

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.

Yeah, I was using the maven name. I agree that we shouldn't add a dependency unless it exists.

I added transaction-coordinator modules and moved theses there

mimaison · 2024-01-12T09:52:09Z

@OmniaGM Can we rebase to resolve the conflicts? Thanks

OmniaGM · 2024-01-15T21:48:41Z

@OmniaGM Can we rebase to resolve the conflicts? Thanks

done now.

mimaison · 2024-01-16T09:43:39Z

@OmniaGM It looks like there are a few build failures:

[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

...coordinator/src/main/java/org/apache/kafka/coordinator/transaction/TransactionLogConfig.java

OmniaGM · 2024-01-16T19:02:21Z

@OmniaGM It looks like there are a few build failures:

[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

group-coordinator/src/main/java/org/apache/kafka/coordinator/group/OffsetConfig.java

checkstyle/import-control.xml

nizhikov · 2024-01-18T16:36:12Z

...coordinator/src/main/java/org/apache/kafka/coordinator/transaction/TransactionLogConfig.java

+ */
+package org.apache.kafka.coordinator.transaction;
+
+public class TransactionLogConfig {


Can we introduce new module and config into separate PR?

it will be very small one which I don't believe it worth it.

mimaison

Thanks for the PR. It looks good overall, I just left a few suggestions

checkstyle/import-control-core.xml

core/src/main/scala/kafka/server/KafkaConfig.scala

core/src/test/scala/other/kafka/StressTestLog.scala

core/src/test/scala/other/kafka/TestLinearWriteSpeed.scala

core/src/test/scala/unit/kafka/cluster/PartitionLockTest.scala

core/src/test/scala/unit/kafka/log/BrokerCompressionTest.scala

core/src/test/scala/unit/kafka/log/LogCleanerManagerTest.scala

core/src/test/scala/unit/kafka/utils/PasswordEncoderTest.scala

server/src/main/java/org/apache/kafka/server/config/Defaults.java

mimaison

LGTM, thanks

ijuma · 2024-01-22T15:35:44Z

server/src/main/java/org/apache/kafka/server/config/Defaults.java

+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+
+public class Defaults {


What's the plan for these defaults, they will not be part of KafkaConfig anymore once that's moved to this module?

They always have been separate object out side of KafkaConfig but in the same file. I just moved them out for reason no. 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.

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).

We can add an exception for checkstyle - it's a tool in the end.

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.

pr opened here to refactor this #15260

Reviewers: Mickael Maison <[email protected]>, Ismael Juma <[email protected]> , David Jacot <[email protected]>, Nikolay <[email protected]>

The uniform assignor got accidentally removed by #15158. This patch adds it back. Reviewers: Omnia G H Ibrahim <[email protected]>, Mickael Maison <[email protected]>

Reviewers: Mickael Maison <[email protected]>, Ismael Juma <[email protected]> , David Jacot <[email protected]>, Nikolay <[email protected]>

The uniform assignor got accidentally removed by apache#15158. This patch adds it back. Reviewers: Omnia G H Ibrahim <[email protected]>, Mickael Maison <[email protected]>

Reviewers: Mickael Maison <[email protected]>, Ismael Juma <[email protected]> , David Jacot <[email protected]>, Nikolay <[email protected]>

The uniform assignor got accidentally removed by apache#15158. This patch adds it back. Reviewers: Omnia G H Ibrahim <[email protected]>, Mickael Maison <[email protected]>

Reviewers: Mickael Maison <[email protected]>, Ismael Juma <[email protected]> , David Jacot <[email protected]>, Nikolay <[email protected]>

The uniform assignor got accidentally removed by apache#15158. This patch adds it back. Reviewers: Omnia G H Ibrahim <[email protected]>, Mickael Maison <[email protected]>

OmniaGM force-pushed the KAFKA-15853-1 branch 2 times, most recently from d2cd220 to 4f86d1e Compare January 9, 2024 14:55

OmniaGM mentioned this pull request Jan 9, 2024

KAFKA-15853: Move KafkaConfig to server module #15103

Closed

5 tasks

nizhikov reviewed Jan 9, 2024

View reviewed changes

core/src/main/scala/kafka/coordinator/group/GroupCoordinator.scala Outdated Show resolved Hide resolved

nizhikov reviewed Jan 9, 2024

View reviewed changes

core/src/main/scala/kafka/server/ClientQuotaManager.scala Outdated Show resolved Hide resolved

nizhikov reviewed Jan 9, 2024

View reviewed changes

core/src/main/scala/kafka/server/ReplicationQuotaManager.scala Outdated Show resolved Hide resolved

This was referenced Jan 9, 2024

KAFKA-15853: Move ClientQuotaManagerConfig outside of core #15159

Merged

KAFKA-15853: Move ReplicationQuotaManagerConfig to server module #15160

Merged

KAFKA-15853: Move OffsetConfig to group-coordinator module #15161

Merged

OmniaGM requested a review from nizhikov January 9, 2024 18:02

OmniaGM mentioned this pull request Jan 9, 2024

KAFKA-15853: Move PasswordEncoder to server module #15164

Closed

3 tasks

dajac reviewed Jan 9, 2024

View reviewed changes

OmniaGM force-pushed the KAFKA-15853-1 branch from 4f86d1e to e5da53c Compare January 11, 2024 15:15

OmniaGM force-pushed the KAFKA-15853-1 branch from e5da53c to bc13fb7 Compare January 15, 2024 21:47

mimaison reviewed Jan 16, 2024

View reviewed changes

...coordinator/src/main/java/org/apache/kafka/coordinator/transaction/TransactionLogConfig.java Outdated Show resolved Hide resolved

OmniaGM added 3 commits January 18, 2024 11:08

KAFKA-15853: Move KafkaConfig.Defaults to server module

3031683

fix checkstyle

917bf94

fix the rebase

e98942c

OmniaGM force-pushed the KAFKA-15853-1 branch from 15ae672 to e98942c Compare January 18, 2024 16:25

nizhikov reviewed Jan 18, 2024

View reviewed changes

group-coordinator/src/main/java/org/apache/kafka/coordinator/group/OffsetConfig.java Outdated Show resolved Hide resolved

nizhikov reviewed Jan 18, 2024

View reviewed changes

checkstyle/import-control.xml Outdated Show resolved Hide resolved

nizhikov reviewed Jan 18, 2024

View reviewed changes

address feedback

bbed2ce

mimaison reviewed Jan 19, 2024

View reviewed changes

address mimaison's feedback

fb4bd58

mimaison approved these changes Jan 22, 2024

View reviewed changes

mimaison merged commit 62ce551 into apache:trunk Jan 22, 2024
1 check failed

ijuma reviewed Jan 22, 2024

View reviewed changes

OmniaGM mentioned this pull request Jan 25, 2024

KAFKA-15853: Refactor org.apache.kafka.server.config.Defaults to follow Java config pattern #15260

Closed

3 tasks

dajac mentioned this pull request Jan 26, 2024

MINOR: Re-add Uniform assignor to the default ones #15269

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-15853: Move KafkaConfig.Defaults to server module #15158

KAFKA-15853: Move KafkaConfig.Defaults to server module #15158

OmniaGM commented Jan 9, 2024 •

edited

Loading

OmniaGM commented Jan 9, 2024

dajac Jan 9, 2024

OmniaGM Jan 9, 2024

ijuma Jan 10, 2024

dajac Jan 10, 2024

ijuma Jan 10, 2024

OmniaGM Jan 11, 2024

mimaison commented Jan 12, 2024

OmniaGM commented Jan 15, 2024

mimaison commented Jan 16, 2024

OmniaGM commented Jan 16, 2024

nizhikov Jan 18, 2024 •

edited

Loading

OmniaGM Jan 18, 2024

mimaison left a comment

mimaison left a comment

ijuma Jan 22, 2024 •

edited

Loading

OmniaGM Jan 22, 2024

ijuma Jan 22, 2024

OmniaGM Jan 24, 2024

OmniaGM Jan 25, 2024

KAFKA-15853: Move KafkaConfig.Defaults to server module #15158

KAFKA-15853: Move KafkaConfig.Defaults to server module #15158

Conversation

OmniaGM commented Jan 9, 2024 • edited Loading

Committer Checklist (excluded from commit message)

OmniaGM commented Jan 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimaison commented Jan 12, 2024

OmniaGM commented Jan 15, 2024

mimaison commented Jan 16, 2024

OmniaGM commented Jan 16, 2024

nizhikov Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimaison left a comment

Choose a reason for hiding this comment

mimaison left a comment

Choose a reason for hiding this comment

ijuma Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmniaGM commented Jan 9, 2024 •

edited

Loading

nizhikov Jan 18, 2024 •

edited

Loading

ijuma Jan 22, 2024 •

edited

Loading