Skip to content

Conversation

@dajac
Copy link
Member

@dajac dajac commented Dec 2, 2025

We had some tests passing null values to
GroupCoordinatorService#onMetadataUpdate even though it is not an
expected case. This patch fixes those tests and ensure that only
non-null values are accepted.

Reviewers: Lianet Magrans lmagrans@confluent.io, Sean Quah
squah@confluent.io, Chia-Ping Tsai chia7712@gmail.com

Copy link
Contributor

@squah-confluent squah-confluent left a comment

Choose a reason for hiding this comment

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

lgtm

@lianetm
Copy link
Member

lianetm commented Dec 2, 2025

Seems we need updates in KRaftCoordinatorMetadataDeltaTest.testKRaftCoordinatorDeltaWithNulls too

@dajac
Copy link
Member Author

dajac commented Dec 3, 2025

Seems we need updates in KRaftCoordinatorMetadataDeltaTest.testKRaftCoordinatorDeltaWithNulls too

@lianetm Fixed it.

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, with one trivial comment left

@Test
public void testKRaftCoordinatorDeltaWithNulls() {
assertTrue(new KRaftCoordinatorMetadataDelta(null).changedTopicIds().isEmpty());
assertTrue(new KRaftCoordinatorMetadataDelta(new MetadataDelta(MetadataImage.EMPTY)).changedTopicIds().isEmpty());
Copy link
Member

Choose a reason for hiding this comment

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

Excuse me, why to remove other test cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

All the tests taking null are irrelevant now and I found the others weird as they are not really related to testing the null case too.

@chia7712 chia7712 merged commit 064dd5f into apache:trunk Dec 3, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants