Skip to content

Conversation

DL1231
Copy link
Contributor

@DL1231 DL1231 commented Sep 1, 2025

  1. Optimize the equals(), hashCode(), and toString() methods in
    OffsetAndMetadata.
  2. Add UT and IT to these modifications.

Reviewers: TengYao Chi [email protected], Sean Quah
[email protected], Chia-Ping Tsai [email protected]

@github-actions github-actions bot added triage PRs from the community consumer clients small Small PRs labels Sep 1, 2025
Copy link
Contributor

@frankvicky frankvicky left a comment

Choose a reason for hiding this comment

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

@DL1231: LGTM

@frankvicky frankvicky requested a review from chia7712 September 2, 2025 15:37
* Normalizes leader epoch values for comparison and hashing.
* Both null and negative values are considered equivalent (representing absent/unknown epoch).
*/
private Integer normalizeLeaderEpoch(Integer leaderEpoch) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Since this method doesn't use any instance members we can make it static
or remove the leaderEpoch parameter.

Comment on lines 102 to 109
/**
* Normalizes leader epoch values for comparison and hashing.
* Both null and negative values are considered equivalent (representing absent/unknown epoch).
*/
private Integer normalizeLeaderEpoch(Integer leaderEpoch) {
return (leaderEpoch == null || leaderEpoch < 0) ? null : leaderEpoch;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the existing leaderEpoch method to normalize the leaderEpoch? It already maps null and negatives to Optional.empty().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.

@github-actions github-actions bot removed the triage PRs from the community label Sep 3, 2025
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.

@DL1231 Thanks for the PR! I think this looks good for item 2 in the JIRA ticket.

I'm not sure what @chia7712 had in mind for items 1 and 3.

  1. OffsetAndMetadata#metadata currently has neither integration nor unit tests

Were we supposed to add tests for metadata null -> "" normalization too?

@chia7712
Copy link
Member

chia7712 commented Sep 3, 2025

had in mind for items 1 and 3.

leaderEpoch=" + leaderEpoch().orElse(null) looks good to me

Were we supposed to add tests for metadata null -> "" normalization too?

+1

assertEquals(5, consumer.committed(Set.of(TP)).get(TP).offset());
metadata = consumer.committed(Set.of(TP)).get(TP);
assertEquals(5, metadata.offset());
assertEquals("metadata", metadata.metadata());
Copy link
Member

Choose a reason for hiding this comment

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

Please update the docs of OffsetAndMetadata#metadata() to remind that the method never return null

@chia7712 chia7712 merged commit 32c2383 into apache:trunk Sep 4, 2025
23 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.

5 participants