KAFKA-15853: Move configDef out of core by OmniaGM · Pull Request #16116 · apache/kafka

OmniaGM · 2024-05-28T22:14:56Z

Moving configDef from core into server::KafkaConfig, keeping the old reference from core::KafkaConfig for the time being to avoid the confusion when we have need to import server::KafkaConfig and core::KafkaConfig.

The following PRs will move the rest of core::KafkaConfig`

This will reduce conflicts while finishing the last part of moving KafkaConfig out of core

Committer Checklist (excluded from commit message)

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

chia7712 · 2024-05-29T21:55:42Z

@OmniaGM Could you please fix the conflicts

…figDef-out

chia7712 · 2024-05-30T13:38:42Z

@OmniaGM Please fix the conflicts, thanks!

chia7712 · 2024-05-30T20:07:52Z

@OmniaGM Sorry that please fix the conflicts again :_

…figDef-out

nizhikov · 2024-06-04T08:28:18Z

There are more conflicts now.

@OmniaGM Can you, please, fix it.

…figDef-out

OmniaGM · 2024-06-04T13:05:26Z

Fixed let's hope this is the last one :)

chia7712 · 2024-06-04T13:13:07Z

Fixed let's hope this is the last one :)

if it can go conflicted, it will :)

chia7712

@OmniaGM thanks for updated PR!

…figDef-out

chia7712 · 2024-06-10T14:31:15Z

+import static org.apache.kafka.common.config.ConfigDef.Type.SHORT;
+import static org.apache.kafka.common.config.ConfigDef.Type.STRING;
+
+public class KafkaConfig {


Could you please add comments to explain why we have "two" KafkaConfig in code base? That can avoid confusion when someone try to add something to KafkaConfig in the future.

- Make org.apache.kafka.server.config.KafkaConfig abstract so others can start move their methods there

chia7712

@OmniaGM thanks for updated PR. could you please fix build error?

[2024-06-10T15:13:31.280Z] > Task :core:compileScala

[2024-06-10T15:13:31.280Z] [Error] /home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-16116/core/src/main/scala/kafka/server/KafkaConfig.scala:27:40: Unused import

chia7712 · 2024-06-11T03:45:42Z

+ * Any code depends on kafka.server.KafkaConfig will keep for using kafka.server.KafkaConfig for the time being until we move it out of core
+ * For more details check KAFKA-15853
+ */
+abstract public class KafkaConfig extends AbstractConfig {


It seems public abstract class is used more frequent than abstract public class. Maybe we can align it?

chia7712 · 2024-06-12T22:28:25Z

@OmniaGM Sorry that conflicts happen again :_

…figDef-out

OmniaGM · 2024-06-13T08:23:17Z

@OmniaGM Sorry that conflicts happen again :_

Done let's hope this is the final one :D

mimaison

LGTM
I left a couple of suggestions but these can be addressed in one of the follow up PRs you have for KafkaConfig

mimaison

LGTM
I left a couple of suggestions but these can be addressed in one of the follow up PRs you have for KafkaConfig

OmniaGM · 2024-06-13T09:51:18Z

I have renamed the class to AbstractKafkaConfig to get ripped of NM_SAME_SIMPLE_NAME_AS_SUPERCLASS error from spotbug

… class

OmniaGM · 2024-06-13T10:12:04Z

That is a intermediate state, right? I assume there will be specific config calss for different component (such as RemoteLogManagerConfig and QuorumConfig). server::KafkaConfig will be either nonexistent or small class (maybe do check for all configs or run once in starting)

Looking into this again. I think we can squeeze part of this vision in as it will reduce conflict with this work going forward specially with the ongoing work for shared groups and new coordinator which I want to avoid as it is hardest to fix. I published this in 88122cb @chia7712 and @mimaison please have a look again. I would really appreciate if we can merge this pr as it is getting painful to keep up with the conflicts :)

For future improvements we can see the possibility of moving getters/ and validations logic as well into these specific config classes but not in the scope of KAFKA-15853.

mimaison · 2024-06-13T15:26:28Z

@OmniaGM spotlessJavaCheck is failing for transaction-coordinator and server-common: https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-16116/17/pipeline

chia7712 · 2024-06-14T00:14:25Z

I think we can squeeze part of this vision in as it will reduce conflict with this work going forward specially with the ongoing work for shared groups and new coordinator which I want to avoid as it is hardest to fix

I love this code style

please have a look again. I would really appreciate if we can merge this pr as it is getting painful to keep up with the conflicts :)

I've set the alarm for this PR :)

chia7712

LGTM

chia7712 · 2024-06-14T02:20:11Z

wait a minute. it seems there are failed tests related to this PR

https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-16116/19/tests

mimaison

This is a bit of a weird state but I understand this is temporary and you have follow up PRs that should tidy things up.
LGTM, let's get this merged as it's a pain to keep in sync

mimaison · 2024-06-14T15:24:32Z

None of the test failures are related, merging to trunk

KAFKA-15853: Move configDef out of KafkaConfig

81cef80

OmniaGM force-pushed the KAFKA-15853-move-configDef-out branch from 51c53bd to 3e580ce Compare May 29, 2024 22:22

Merge remote-tracking branch 'apache/trunk' into KAFKA-15853-move-con…

3c70661

…figDef-out

OmniaGM force-pushed the KAFKA-15853-move-configDef-out branch from 3e580ce to 3c70661 Compare May 29, 2024 22:27

Merge remote-tracking branch 'apache/trunk' into KAFKA-15853-move-con…

1a765a5

…figDef-out

OmniaGM force-pushed the KAFKA-15853-move-configDef-out branch from 753ecbb to 1a765a5 Compare May 29, 2024 22:34

Merge remote-tracking branch 'apache/trunk' into KAFKA-15853-move-con…

9aef77c

…figDef-out

OmniaGM force-pushed the KAFKA-15853-move-configDef-out branch from 72c3776 to 9aef77c Compare June 3, 2024 08:46

Merge remote-tracking branch 'apache/trunk' into KAFKA-15853-move-con…

03b33ff

…figDef-out

chia7712 reviewed Jun 5, 2024

View reviewed changes

Comment thread server/src/main/java/org/apache/kafka/server/config/KafkaConfig.java Outdated

chia7712 reviewed Jun 9, 2024

View reviewed changes

Comment thread server/src/main/java/org/apache/kafka/server/config/KafkaConfig.java Outdated

Comment thread server/src/main/java/org/apache/kafka/server/config/KafkaConfig.java Outdated

OmniaGM added 2 commits June 9, 2024 23:01

add RemoteLogManagerConfig.CONFIG_DEF

6847fd7

Merge remote-tracking branch 'apache/trunk' into KAFKA-15853-move-con…

bed0324

…figDef-out

chia7712 reviewed Jun 10, 2024

View reviewed changes

add commit to explain the diff between the two kafka configs

132f5ec

- Make org.apache.kafka.server.config.KafkaConfig abstract so others can start move their methods there

chia7712 reviewed Jun 11, 2024

View reviewed changes

feedback

46a8d6a

OmniaGM mentioned this pull request Jun 12, 2024

KAFKA-15853: Move config getters and validation into server #16307

Closed

3 tasks

Merge remote-tracking branch 'apache/trunk' into KAFKA-15853-move-con…

0741ad5

…figDef-out

fix

6362f5a

mimaison approved these changes Jun 13, 2024

View reviewed changes

Comment thread server/src/main/java/org/apache/kafka/server/config/KafkaConfig.java Outdated

mimaison approved these changes Jun 13, 2024

View reviewed changes

Comment thread server/src/main/java/org/apache/kafka/server/config/KafkaConfig.java Outdated

OmniaGM added 2 commits June 13, 2024 10:15

address feedback

bb7881b

rename org.apache.kafka.server.config.KafkaConfig to AbstractKafkaConfig

8b1b7db

Break CONFIG_DEF into multi def each one defined in its own Config(s)…

88122cb

… class

OmniaGM mentioned this pull request Jun 13, 2024

KAFKA-16638 Align the naming convention for config and default variables in *Config classes #16289

Closed

3 tasks

fix pipeline

5389335

OmniaGM force-pushed the KAFKA-15853-move-configDef-out branch from 7c173f7 to 5389335 Compare June 13, 2024 20:55

chia7712 approved these changes Jun 14, 2024

View reviewed changes

chia7712 reviewed Jun 14, 2024

View reviewed changes

Comment thread ...oordinator/src/main/java/org/apache/kafka/coordinator/transaction/TransactionLogConfigs.java

add missing LOG_DIR_FAILURE_TIMEOUT_MS_CONFIG

f73c1cc

mimaison approved these changes Jun 14, 2024

View reviewed changes

mimaison merged commit e99da24 into apache:trunk Jun 14, 2024

Conversation

OmniaGM commented May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

chia7712 commented May 29, 2024

Uh oh!

chia7712 commented May 30, 2024

Uh oh!

chia7712 commented May 30, 2024

Uh oh!

nizhikov commented Jun 4, 2024

Uh oh!

OmniaGM commented Jun 4, 2024

Uh oh!

chia7712 commented Jun 4, 2024

Uh oh!

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chia7712 Jun 10, 2024

Choose a reason for hiding this comment

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

chia7712 Jun 11, 2024

Choose a reason for hiding this comment

Uh oh!

chia7712 commented Jun 12, 2024

Uh oh!

OmniaGM commented Jun 13, 2024

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

OmniaGM commented Jun 13, 2024

Uh oh!

OmniaGM commented Jun 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mimaison commented Jun 13, 2024

Uh oh!

chia7712 commented Jun 14, 2024

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

chia7712 commented Jun 14, 2024

Uh oh!

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

mimaison commented Jun 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

OmniaGM commented May 28, 2024 •

edited

Loading

OmniaGM commented Jun 13, 2024 •

edited

Loading