Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-17477:Topic command integration test migrate to new test infra #16127

Merged
merged 25 commits into from
Sep 8, 2024

Conversation

TaiJuWu
Copy link
Contributor

@TaiJuWu TaiJuWu commented May 29, 2024

*More detailed description of your change,

  1. New interface aliveBrokers() for clusterInstance.
  2. Migrate topic command integration test migrate to new test infra

*Summary of testing strategy (including rationale)

Committer Checklist (excluded from commit message)

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

@TaiJuWu TaiJuWu marked this pull request as draft May 29, 2024 18:29
@gharris1727 gharris1727 added the tests Test fixes (including flaky tests) label May 31, 2024
@TaiJuWu TaiJuWu force-pushed the TopicComIT-migrate branch 6 times, most recently from 3ced55a to fff462f Compare June 8, 2024 07:36
@TaiJuWu TaiJuWu changed the title [Draft]Topic command integration test migrate to new test infra MINOR:Topic command integration test migrate to new test infra Jun 8, 2024
@TaiJuWu TaiJuWu marked this pull request as ready for review June 8, 2024 07:51
Copy link
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.

@TaiJuWu thanks for this patch

core/src/test/java/kafka/testkit/KafkaClusterTestKit.java Outdated Show resolved Hide resolved
core/src/test/java/kafka/testkit/KafkaClusterTestKit.java Outdated Show resolved Hide resolved
@chia7712
Copy link
Member

@TaiJuWu please rebase code

@TaiJuWu
Copy link
Contributor Author

TaiJuWu commented Jun 22, 2024

@TaiJuWu please rebase code

Hi @chia7712 , I have update this PR and clear commits, PTAL.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@TaiJuWu TaiJuWu force-pushed the TopicComIT-migrate branch 4 times, most recently from 2e12dd1 to 4583d28 Compare June 22, 2024 11:15
TaiJuWu added 4 commits June 22, 2024 22:13
Old testcase use PLAINTEXT as its protocol, I try to minimize this PR
so make TestKit support PLAINTEXT protocol.
@TaiJuWu TaiJuWu force-pushed the TopicComIT-migrate branch from 488ccce to dc6a7af Compare June 22, 2024 14:16
@chia7712
Copy link
Member

@TaiJuWu please take a look at unrelated changes.

Copy link
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.

@TaiJuWu thanks for your patch

@TaiJuWu TaiJuWu force-pushed the TopicComIT-migrate branch from 395beef to d312323 Compare July 6, 2024 02:14
@chia7712
Copy link
Member

chia7712 commented Aug 6, 2024

@TaiJuWu Could you please rebase code to trigger QA again?

@TaiJuWu
Copy link
Contributor Author

TaiJuWu commented Aug 6, 2024

@TaiJuWu Could you please rebase code to trigger QA again?

Done. Thanks!

Copy link
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.

@TaiJuWu thanks for this patch

Copy link
Collaborator

@OmniaGM OmniaGM left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, I had a quick Look and left few comments

@OmniaGM OmniaGM added the tools label Aug 7, 2024
@chia7712
Copy link
Member

@TaiJuWu Could you please fix the conflicts?

@TaiJuWu TaiJuWu changed the title MINOR:Topic command integration test migrate to new test infra KAFKA-17477:Topic command integration test migrate to new test infra Sep 4, 2024
@TaiJuWu
Copy link
Contributor Author

TaiJuWu commented Sep 5, 2024

@TaiJuWu Could you please fix the conflicts?

Hi @chia7712 , thanks for review, update!

@TaiJuWu TaiJuWu force-pushed the TopicComIT-migrate branch 2 times, most recently from be62d75 to f809d4b Compare September 5, 2024 08:12
@SuppressWarnings("deprecation") // Added for Scala 2.12 compatibility for usages of JavaConverters
public class TopicCommandIntegrationTest extends kafka.integration.KafkaServerTestHarness implements Logging, RackAwareTest {
@ExtendWith(ClusterTestExtensions.class)
public class TopicCommandIntegrationTest {
Copy link
Member

Choose a reason for hiding this comment

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

@TaiJuWu Could you please rename it to TopicCommandTest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
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.

@TaiJuWu thanks for your patch

Map<Integer, Integer> partitionCount = new HashMap<>();
Map<Integer, List<String>> partitionRackMap = new HashMap<>();

assignment.entrySet().stream().forEach(entry -> {
Copy link
Member

Choose a reason for hiding this comment

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

        assignment.forEach((partitionId, replicaList) -> {
            Integer leader = replicaList.get(0);
            leaderCount.put(leader, leaderCount.getOrDefault(leader, 0) + 1);
            replicaList.forEach(brokerId -> {
                partitionCount.put(brokerId, partitionCount.getOrDefault(brokerId, 0) + 1);
                String rack;
                if (brokerRackMapping.containsKey(brokerId)) {
                    rack = brokerRackMapping.get(brokerId);
                    List<String> partitionRackValues = Stream.of(Collections.singletonList(rack), partitionRackMap.getOrDefault(partitionId, Collections.emptyList()))
                            .flatMap(List::stream)
                            .collect(Collectors.toList());
                    partitionRackMap.put(partitionId, partitionRackValues);
                } else {
                    System.err.printf("No mapping found for %s in `brokerRackMapping`%n", brokerId);
                }
            });
        });

Boolean verifyLeaderDistribution,
Boolean verifyReplicasDistribution) {
// always verify that no broker will be assigned for more than one replica
assignment.entrySet().stream()
Copy link
Member

Choose a reason for hiding this comment

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

        assignment.forEach((key, value1) -> assertEquals(new HashSet<>(value1).size(), value1.size(),
                "More than one replica is assigned to same broker for the same partition"));

@chia7712 chia7712 merged commit 04dee3b into apache:trunk Sep 8, 2024
5 of 6 checks passed
@TaiJuWu TaiJuWu deleted the TopicComIT-migrate branch September 10, 2024 23:30
tedyu pushed a commit to tedyu/kafka that referenced this pull request Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Test fixes (including flaky tests) tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants