Skip to content

KAFKA-16643 Add ModifierOrder checkstyle rule#15890

Merged
chia7712 merged 8 commits intoapache:trunkfrom
gongxuanzhang:fix_chaos_modifier
Jun 13, 2024
Merged

KAFKA-16643 Add ModifierOrder checkstyle rule#15890
chia7712 merged 8 commits intoapache:trunkfrom
gongxuanzhang:fix_chaos_modifier

Conversation

@gongxuanzhang
Copy link
Copy Markdown
Contributor

@gongxuanzhang gongxuanzhang commented May 8, 2024

@gharris1727 #15812
I modified all the non-standard code according to the rules of ModifierOrder, and I divided it into multiple commits by module for better code review
Additional,I modified a scala file to follow the java specification.I don't know if that's right

@gongxuanzhang
a slight change.
f4e3461 
The modifiers should be in uniform order
@gongxuanzhang gongxuanzhang mentioned this pull request May 8, 2024
Closed
@chia7712
Copy link
Copy Markdown
Member

chia7712 commented May 8, 2024

@gongxuanzhang thanks for this patch. Could you take a look at https://issues.apache.org/jira/browse/KAFKA-10787? Personally, we should introduce the new checkstyle file. And then we apply it by module to reduce the patch size.

@gongxuanzhang
Copy link
Copy Markdown
Contributor Author

gongxuanzhang commented May 8, 2024

@chia7712 Yes i do .
I think these changes, though numerous, are still simple changes.
Even with the introduction of new checkstyle file , the style we pursue is always "unity".
In this PR I have followed the checkstyle ModifierOrder rule and passed the self-test.
Thank you very much for your reply and I hope I can make some contribution to kafka

@chia7712
Copy link
Copy Markdown
Member

chia7712 commented May 8, 2024

In this PR I have followed the checkstyle ModifierOrder rule and passed the self-test.

We need the checkstyle first, because it is hard to review those huge changes by human eyes 😀

Thank you very much for your reply and I hope I can make some contribution to kafka

Welcome to Kafka community. Please try to add new checkstyle import rules to small module. I'd like to review them for you 😄

@gongxuanzhang
Copy link
Copy Markdown
Contributor Author

gongxuanzhang commented May 8, 2024

@chia7712
I will resubmit my PR, introduce the new checkstyle and re-modify the PR according to the module.
Thank you for your patience and best wishes

@gongxuanzhang
Copy link
Copy Markdown
Contributor Author

gongxuanzhang commented May 10, 2024 •
edited
Loading

@chia7712
I add rule into checkstyle.xml

<module name="ModifierOrder"/>

you can run

./gradlew checkstyleMain checkstyleTest --continue

@chia7712
Copy link
Copy Markdown
Member

chia7712 commented May 10, 2024

@gongxuanzhang This is still a huge patch, which contains a log of changes across all modules. This is a important improvement which can impact all Kafka developers, and so we should separate it to small PRs.

Personally, the first patch should include 1) new checkstyle and 2) auto formatter ( see https://issues.apache.org/jira/browse/KAFKA-12572)

After first patch gets merged, we can apply the rule to all modules "one by one". Yes, it needs many PRs but they make reviews workable.

@gongxuanzhang
Copy link
Copy Markdown
Contributor Author

gongxuanzhang commented May 10, 2024 •
edited
Loading

@chia7712
I get it!
But I'm confused. Is there anything I can do about it

@chia7712
Copy link
Copy Markdown
Member

chia7712 commented May 10, 2024

But I'm confused. Is there anything I can do about it

@dongjinleekr had filed a PR (#10428) to be the start. However, the PR gets conflicts now, and not sure whether @dongjinleekr has free cycles to address it. You can file a new PR to address that.

@chia7712
Copy link
Copy Markdown
Member

chia7712 commented Jun 12, 2024

@gongxuanzhang could you fix the conflicts? Also, please revert the unrelated changes. thanks!

@gongxuanzhang gongxuanzhang force-pushed the fix_chaos_modifier branch 3 times, most recently from a7432df to f4e3461 Compare June 12, 2024 10:00
gongxuanzhang and others added 4 commits June 12, 2024 22:44
@gongxuanzhang
update modifier order and add checkstyle rule
9df42fa
@gongxuanzhang
Merge branch 'trunk' into fix_chaos_modifier
a1922eb 
# Conflicts:
#	clients/src/test/java/org/apache/kafka/clients/admin/MockAdminClient.java
#	group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorConfig.java
#	group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTestContext.java
#	metadata/src/test/java/org/apache/kafka/image/node/TopicImageNodeTest.java
#	metadata/src/test/java/org/apache/kafka/metadata/bootstrap/BootstrapMetadataTest.java
#	raft/src/main/java/org/apache/kafka/snapshot/RecordsSnapshotWriter.java
#	server/src/main/java/org/apache/kafka/server/config/KafkaSecurityConfigs.java
#	streams/src/test/java/org/apache/kafka/streams/processor/internals/DefaultStateUpdaterTest.java
#	streams/src/test/java/org/apache/kafka/streams/processor/internals/TasksTest.java
@gongxuanzhang
update modifier order and add checkstyle rule
006c3d8
@gongxuanzhang
Merge branch 'apache:trunk' into fix_chaos_modifier
f4f1267
@chia7712
Copy link
Copy Markdown
Member

chia7712 commented Jun 12, 2024

@gongxuanzhang please fix the build error

@chia7712 chia7712 changed the title KAFKA-16643 Fix chaos modifier KAFKA-16643 Add ModifierOrder checkstyle rule Jun 13, 2024
chia7712
chia7712 approved these changes Jun 13, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chia7712 chia7712 merged commit 596b945 into apache:trunk Jun 13, 2024
apourchet added a commit to apourchet/kafka that referenced this pull request Jun 13, 2024
@apourchet
Squashed commit of the following:
be0edb2 
commit f380cd1
Author: Edoardo Comar <ecomar@uk.ibm.com>
Date:   Thu Jun 13 15:01:08 2024 +0100

    MINOR: Add integration tag to AdminFenceProducersIntegrationTest (apache#16326)

    Add @tag("integration") to AdminFenceProducersIntegrationTest

    Reviewers: Chris Egerton <chrise@aiven.io>

commit 11c85a9
Author: Dongnuo Lyu <139248811+dongnuo123@users.noreply.github.com>
Date:   Thu Jun 13 05:11:01 2024 -0400

    MINOR: Make online downgrade failure logs less noisy and update the timeouts scheduled in `convertToConsumerGroup` (apache#16290)

    This patch:
    - changes the order of the checks in `validateOnlineDowngrade`, so that only when the last member using the consumer protocol leave and the group still has classic member(s), `online downgrade is disabled` is logged if the policy doesn't allow downgrade.
    - changes the session timeout in `convertToConsumerGroup` from `consumerGroupSessionTimeoutMs` to `member.classicProtocolSessionTimeout().get()`.

    Reviewers: David Jacot <djacot@confluent.io>

commit ea60666
Author: Ken Huang <100591800+m1a2st@users.noreply.github.com>
Date:   Thu Jun 13 17:11:37 2024 +0900

    KAFKA-16921 [1/N] Migrate all junit 4 code to junit 5 for connect module (apache#16253)

    Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

commit 596b945
Author: gongxuanzhang <gongxuanzhang@foxmail.com>
Date:   Thu Jun 13 15:39:32 2024 +0800

    KAFKA-16643 Add ModifierOrder checkstyle rule (apache#15890)

    Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

commit 103ff5c
Author: Antoine Pourchet <antoine@responsive.dev>
Date:   Thu Jun 13 01:32:39 2024 -0600

    KAFKA-15045: (KIP-924 pt. 24) internal TaskAssignor rename to LegacyTaskAssignor (apache#16318)

    Since the new public API for TaskAssignor shared a name, this rename will prevent users from confusing the internal definition with the public one.

    Reviewers: Anna Sophie Blee-Goldman <ableegoldman@apache.org>

commit e59c887
Author: brenden20 <118419078+brenden20@users.noreply.github.com>
Date:   Thu Jun 13 02:30:05 2024 -0500

    KAFKA-16557 Implemented OffsetFetchRequestState toStringBase and added a test for it (apache#16291)

    Reviewers: Lianet Magrans <lianetmr@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>

commit dd6fcc6
Author: TingIāu "Ting" Kì <kitingiao@gmail.com>
Date:   Thu Jun 13 14:35:33 2024 +0800

    KAFKA-16901 Add unit tests for ConsumerRecords#records(String) (apache#16227)

    Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

commit fe98888
Author: Lianet Magrans <98415067+lianetm@users.noreply.github.com>
Date:   Thu Jun 13 08:31:16 2024 +0200

    MINOR: Improving log for outstanding requests on close and cleanup (apache#16304)

    Reviewers: Andrew Schofield <aschofield@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com>

commit 9ddd58b
Author: Chris Egerton <chrise@aiven.io>
Date:   Thu Jun 13 05:43:33 2024 +0200

    MINOR: Add readiness check for connector and separate Kafka cluster in ExactlyOnceSourceIntegrationTest::testSeparateOffsetsTopic (apache#16306)

    Reviewers: Greg Harris <gharris1727@gmail.com>

commit 0a203a9
Author: TingIāu "Ting" Kì <kitingiao@gmail.com>
Date:   Thu Jun 13 09:47:51 2024 +0800

    KAFKA-16938 non-dynamic props gets corrupted due to circular reference between DynamicBrokerConfig and DynamicConfig. (apache#16302)

    Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

commit 6d1f8f8
Author: Gantigmaa Selenge <39860586+tinaselenge@users.noreply.github.com>
Date:   Thu Jun 13 02:42:39 2024 +0100

    MINOR: Clean up for KafkaAdminClientTest (apache#16285)

    Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

commit e76e1da
Author: Chris Egerton <chrise@aiven.io>
Date:   Thu Jun 13 02:18:23 2024 +0200

    KAFKA-16935: Automatically wait for cluster startup in embedded Connect integration tests (apache#16288)

    Reviewers: Greg Harris <gharris1727@gmail.com>
apourchet added a commit to apourchet/kafka that referenced this pull request Jun 13, 2024
@apourchet
Squashed commit of the following:
61700fe 
commit 4333af5
Author: A. Sophie Blee-Goldman <ableegoldman@gmail.com>
Date:   Thu Jun 13 11:27:50 2024 -0700

    KAFKA-15045: (KIP-924 pt. 25) Rename old internal StickyTaskAssignor to LegacyStickyTaskAssignor (apache#16322)

    To avoid confusion in 3.8/until we fully remove all the old task assignors and internal config, we should rename the old internal assignor classes like the StickyTaskAssignor so that they won't be mixed up with the new version of the assignor (which is also named StickyTaskAssignor)

    Reviewers: Bruno Cadonna <cadonna@apache.org>, Josep Prat <josep.prat@aiven.io>

commit f380cd1
Author: Edoardo Comar <ecomar@uk.ibm.com>
Date:   Thu Jun 13 15:01:08 2024 +0100

    MINOR: Add integration tag to AdminFenceProducersIntegrationTest (apache#16326)

    Add @tag("integration") to AdminFenceProducersIntegrationTest

    Reviewers: Chris Egerton <chrise@aiven.io>

commit 11c85a9
Author: Dongnuo Lyu <139248811+dongnuo123@users.noreply.github.com>
Date:   Thu Jun 13 05:11:01 2024 -0400

    MINOR: Make online downgrade failure logs less noisy and update the timeouts scheduled in `convertToConsumerGroup` (apache#16290)

    This patch:
    - changes the order of the checks in `validateOnlineDowngrade`, so that only when the last member using the consumer protocol leave and the group still has classic member(s), `online downgrade is disabled` is logged if the policy doesn't allow downgrade.
    - changes the session timeout in `convertToConsumerGroup` from `consumerGroupSessionTimeoutMs` to `member.classicProtocolSessionTimeout().get()`.

    Reviewers: David Jacot <djacot@confluent.io>

commit ea60666
Author: Ken Huang <100591800+m1a2st@users.noreply.github.com>
Date:   Thu Jun 13 17:11:37 2024 +0900

    KAFKA-16921 [1/N] Migrate all junit 4 code to junit 5 for connect module (apache#16253)

    Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

commit 596b945
Author: gongxuanzhang <gongxuanzhang@foxmail.com>
Date:   Thu Jun 13 15:39:32 2024 +0800

    KAFKA-16643 Add ModifierOrder checkstyle rule (apache#15890)

    Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

commit 103ff5c
Author: Antoine Pourchet <antoine@responsive.dev>
Date:   Thu Jun 13 01:32:39 2024 -0600

    KAFKA-15045: (KIP-924 pt. 24) internal TaskAssignor rename to LegacyTaskAssignor (apache#16318)

    Since the new public API for TaskAssignor shared a name, this rename will prevent users from confusing the internal definition with the public one.

    Reviewers: Anna Sophie Blee-Goldman <ableegoldman@apache.org>

commit e59c887
Author: brenden20 <118419078+brenden20@users.noreply.github.com>
Date:   Thu Jun 13 02:30:05 2024 -0500

    KAFKA-16557 Implemented OffsetFetchRequestState toStringBase and added a test for it (apache#16291)

    Reviewers: Lianet Magrans <lianetmr@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>

commit dd6fcc6
Author: TingIāu "Ting" Kì <kitingiao@gmail.com>
Date:   Thu Jun 13 14:35:33 2024 +0800

    KAFKA-16901 Add unit tests for ConsumerRecords#records(String) (apache#16227)

    Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

commit fe98888
Author: Lianet Magrans <98415067+lianetm@users.noreply.github.com>
Date:   Thu Jun 13 08:31:16 2024 +0200

    MINOR: Improving log for outstanding requests on close and cleanup (apache#16304)

    Reviewers: Andrew Schofield <aschofield@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com>

commit 9ddd58b
Author: Chris Egerton <chrise@aiven.io>
Date:   Thu Jun 13 05:43:33 2024 +0200

    MINOR: Add readiness check for connector and separate Kafka cluster in ExactlyOnceSourceIntegrationTest::testSeparateOffsetsTopic (apache#16306)

    Reviewers: Greg Harris <gharris1727@gmail.com>

commit 0a203a9
Author: TingIāu "Ting" Kì <kitingiao@gmail.com>
Date:   Thu Jun 13 09:47:51 2024 +0800

    KAFKA-16938 non-dynamic props gets corrupted due to circular reference between DynamicBrokerConfig and DynamicConfig. (apache#16302)

    Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

commit 6d1f8f8
Author: Gantigmaa Selenge <39860586+tinaselenge@users.noreply.github.com>
Date:   Thu Jun 13 02:42:39 2024 +0100

    MINOR: Clean up for KafkaAdminClientTest (apache#16285)

    Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

commit e76e1da
Author: Chris Egerton <chrise@aiven.io>
Date:   Thu Jun 13 02:18:23 2024 +0200

    KAFKA-16935: Automatically wait for cluster startup in embedded Connect integration tests (apache#16288)

    Reviewers: Greg Harris <gharris1727@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chia7712 chia7712 chia7712 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@gongxuanzhang @chia7712