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.

Sign up for GitHub

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

Jump to bottom

KAFKA-16922 : Adding unit tests for NewTopic #16255

Merged
merged 4 commits into from
Jun 12, 2024
Merged

KAFKA-16922 : Adding unit tests for NewTopic #16255

merged 4 commits into from
Jun 12, 2024

Conversation

muralibasani
Copy link
Contributor

Resolves https://issues.apache.org/jira/browse/KAFKA-16922

Adding unit tests for org.apache.kafka.clients.admin.NewTopic

Committer Checklist (excluded from commit message)

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

@muralibasani
Copy link
Contributor Author

@chia7712 can you pls take a look ?

chia7712
chia7712 reviewed Jun 9, 2024
View reviewed 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.

@muralibasani thanks for offering so many great test cases!

clients/src/test/java/org/apache/kafka/clients/admin/NewTopicTest.java
CreateTopicsRequestData.CreatableTopic creatableTopic = topic.convertToCreatableTopic();

assertEquals(TEST_TOPIC, creatableTopic.name());
assertEquals(NO_NUM_PARTITIONS, creatableTopic.numPartitions());
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add test case that CreateTopicsRequestData.CreatableTopic#numPartitions and CreateTopicsRequestData.CreatableTopic#replicationFactor return positive value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, added one.

clients/src/test/java/org/apache/kafka/clients/admin/NewTopicTest.java
@Test
public void testConfigs() {
NewTopic newTopic = new NewTopic(TEST_TOPIC, NUM_PARTITIONS, (short) REPLICATION_FACTOR);
Map<String, String> configs = new HashMap<>();
Copy link
Member

Choose a reason for hiding this comment

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

Could you verify that newTopic.configs() will return null if it has no configs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, added one.

clients/src/test/java/org/apache/kafka/clients/admin/NewTopicTest.java Outdated

public static final String TEST_TOPIC = "testtopic";
public static final int NUM_PARTITIONS = 3;
public static final int REPLICATION_FACTOR = 1;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can change the type from int to short?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, updated.

@muralibasani muralibasani requested a review from chia7712 June 9, 2024 19:04
@muralibasani
Copy link
Contributor Author

@chia7712 looks like the failed tests in the ci are unrelated.

chia7712
chia7712 reviewed Jun 11, 2024
View reviewed 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.

@muralibasani thanks for this patch. those tests are great to me. The last two comments are left. PTAL

clients/src/test/java/org/apache/kafka/clients/admin/NewTopicTest.java Outdated
NewTopic newTopic = new NewTopic(TEST_TOPIC, replicasAssignments);
Map<Integer, List<Integer>> returnedAssignments = newTopic.replicasAssignments();

assertThrows(UnsupportedOperationException.class, () -> {
Copy link
Member

Choose a reason for hiding this comment

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

it seems this can be simplified to assertThrows(UnsupportedOperationException.class, () -> returnedAssignments.put(1, Arrays.asList(3, 4)));

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.

clients/src/test/java/org/apache/kafka/clients/admin/NewTopicTest.java Outdated
public void testToString() {
NewTopic topic = new NewTopic(TEST_TOPIC, NUM_PARTITIONS, REPLICATION_FACTOR);
String expected = "(name=" + TEST_TOPIC + ", numPartitions=" + NUM_PARTITIONS + ", replicationFactor=" + REPLICATION_FACTOR + ", replicasAssignments=null, configs=null)";
assertEquals(expected, topic.toString());
Copy link
Member

Choose a reason for hiding this comment

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

maybe we can verify the non-null configs/assignments too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice one, updated.

@muralibasani muralibasani requested a review from chia7712 June 11, 2024 13:49
chia7712
chia7712 approved these changes Jun 11, 2024
View reviewed 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.

LGTM

@chia7712 chia7712 merged commit 226ac5e into apache:trunk Jun 12, 2024
1 check failed
@chia7712
Copy link
Member

@muralibasani thanks for this nice test!

@muralibasani
Copy link
Contributor Author

@chia7712 thank you for the great review.

gongxuanzhang pushed a commit to gongxuanzhang/kafka that referenced this pull request Jun 12, 2024
@muralibasani @gongxuanzhang
KAFKA-16922 Adding unit tests for NewTopic (apache#16255)
1695e6e 
Reviewers: Chia-Ping Tsai <[email protected]>
apourchet added a commit to apourchet/kafka that referenced this pull request Jun 12, 2024
@apourchet
Squashed commit of the following:
4ba836d 
commit 9368ef8
Author: Gantigmaa Selenge <[email protected]>
Date:   Wed Jun 12 16:04:24 2024 +0100

    KAFKA-16865: Add IncludeTopicAuthorizedOperations option for DescribeTopicPartitionsRequest (apache#16136)

    Reviewers: Mickael Maison <[email protected]>, Chia-Ping Tsai <[email protected]>, Calvin Liu <[email protected]>, Andrew Schofield <[email protected]>, Apoorv Mittal <[email protected]>

commit 46eb081
Author: gongxuanzhang <[email protected]>
Date:   Wed Jun 12 22:23:39 2024 +0800

    KAFKA-10787 Apply spotless to log4j-appender, trogdor, jmh-benchmarks, examples, shell and generator (apache#16296)

    Reviewers: Chia-Ping Tsai <[email protected]>

commit 79b9c44
Author: gongxuanzhang <[email protected]>
Date:   Wed Jun 12 22:19:47 2024 +0800

    KAFKA-10787 Apply spotless to connect module (apache#16299)

    Reviewers: Chia-Ping Tsai <[email protected]>

commit b5fb654
Author: Abhijeet Kumar <[email protected]>
Date:   Wed Jun 12 19:47:46 2024 +0530

    KAFKA-15265: Dynamic broker configs for remote fetch/copy quotas (apache#16078)

    Reviewers: Kamal Chandraprakash<[email protected]>, Satish Duggana <[email protected]>

commit faee6a4
Author: Dmitry Werner <[email protected]>
Date:   Wed Jun 12 15:44:11 2024 +0500

    MINOR: Use predetermined dir IDs in ReplicationQuotasTest

    Use predetermined directory IDs instead of Uuid.randomUuid() in ReplicationQuotasTest.

    Reviewers: Igor Soarez <[email protected]>

commit 638844f
Author: David Jacot <[email protected]>
Date:   Wed Jun 12 08:29:50 2024 +0200

    KAFKA-16770; [2/2] Coalesce records into bigger batches (apache#16215)

    This patch is the continuation of apache#15964. It introduces the records coalescing to the CoordinatorRuntime. It also introduces a new configuration `group.coordinator.append.linger.ms` which allows administrators to chose the linger time or disable it with zero. The new configuration defaults to 10ms.

    Reviewers: Jeff Kim <[email protected]>, Justine Olshan <[email protected]>

commit 39ffdea
Author: Bruno Cadonna <[email protected]>
Date:   Wed Jun 12 07:51:38 2024 +0200

    KAFKA-10199: Enable state updater by default (apache#16107)

    We have already enabled the state updater by default once.
    However, we ran into issues that forced us to disable it again.
    We think that we fixed those issues. So we want to enable the
    state updater again by default.

    Reviewers: Lucas Brutschy <[email protected]>, Matthias J. Sax <[email protected]>

commit 0782232
Author: Antoine Pourchet <[email protected]>
Date:   Tue Jun 11 22:31:43 2024 -0600

    KAFKA-15045: (KIP-924 pt. 22) Add RackAwareOptimizationParams and other minor TaskAssignmentUtils changes (apache#16294)

    We now provide a way to more easily customize the rack aware
    optimizations that we provide by way of a configuration class called
    RackAwareOptimizationParams.

    We also simplified the APIs for the optimizeXYZ utility functions since
    they were mutating the inputs anyway.

    Reviewers: Anna Sophie Blee-Goldman <[email protected]>

commit 226ac5e
Author: Murali Basani <[email protected]>
Date:   Wed Jun 12 05:38:50 2024 +0200

    KAFKA-16922 Adding unit tests for NewTopic  (apache#16255)

    Reviewers: Chia-Ping Tsai <[email protected]>

commit 23fe71d
Author: Abhijeet Kumar <[email protected]>
Date:   Wed Jun 12 06:27:02 2024 +0530

    KAFKA-15265: Integrate RLMQuotaManager for throttling copies to remote storage (apache#15820)

    - Added the integration of the quota manager to throttle copy requests to the remote storage. Reference KIP-956
    - Added unit-tests for the copy throttling logic.

    Reviewers: Satish Duggana <[email protected]>, Luke Chen <[email protected]>, Kamal Chandraprakash<[email protected]>

commit 2fa2c72
Author: Chris Egerton <[email protected]>
Date:   Tue Jun 11 23:15:07 2024 +0200

    MINOR: Wait for embedded clusters to start before using them in Connect OffsetsApiIntegrationTest (apache#16286)

    Reviewers: Greg Harris <[email protected]>
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
@muralibasani @chia7712