Skip to content

Conversation

@viirya
Copy link
Member

@viirya viirya commented Oct 30, 2025

What changes were proposed in this pull request?

This patch extracts the same method getOffsetRangesFromResolvedOffsets from two KafkaOffsetReader implementations.

Why are the changes needed?

When reviewing #52729, found that KafkaOffsetReaderConsumer and KafkaOffsetReaderAdmin have the exactly same getOffsetRangesFromResolvedOffsets method. The method is actually long so seems good to extract them to common one.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing tests.

Was this patch authored or co-authored using generative AI tooling?

No

@viirya
Copy link
Member Author

viirya commented Oct 30, 2025

When reviewing #52729, found that KafkaOffsetReaderConsumer and KafkaOffsetReaderAdmin have the exactly same getOffsetRangesFromResolvedOffsets method. The method is actually long so seems good to extract them to common one.

@viirya viirya changed the title [SPARK-XXXXX][SQL] Extract common methods to KafkaOffsetReaderBase [MINOR][SQL] Extract common methods to KafkaOffsetReaderBase Oct 30, 2025
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Given the size of code, we need JIRA ID for this PR, @viirya . 😄

@viirya
Copy link
Member Author

viirya commented Oct 30, 2025

Ha, okay @dongjoon-hyun 😄

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM (with the dedicated JIRA ID and a few style comments).

@viirya viirya changed the title [MINOR][SQL] Extract common methods to KafkaOffsetReaderBase [SPARK-54094][SQL] Extract common methods to KafkaOffsetReaderBase Oct 30, 2025
@viirya viirya closed this in 7c5a9a3 Oct 30, 2025
@viirya viirya deleted the kafkaoffsetreader_refactor branch October 30, 2025 21:04
@viirya
Copy link
Member Author

viirya commented Oct 30, 2025

Thank you @dongjoon-hyun

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.

2 participants