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

[SPARK-50360][SS][FOLLOWUP][MINOR] Make readVersion lazy value #49091

Closed
wants to merge 4 commits into from

Conversation

tedyu
Copy link
Contributor

@tedyu tedyu commented Dec 6, 2024

What changes were proposed in this pull request?

This is followup to #48880
readVersion is made a lazy value.
After input is closed, its value is not changed to null because input is only used by readVersion.

Why are the changes needed?

StateStoreChangelogReaderFactory is able to produce more than one reader.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing test suite.

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

No

@tedyu
Copy link
Contributor Author

tedyu commented Dec 6, 2024

cc @WweiL @HeartSaVioR

Copy link
Contributor

@WweiL WweiL left a comment

Choose a reason for hiding this comment

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

+1 pending tests, thanks for the follow up!

@tedyu tedyu changed the title [MINOR] Assign null to input after closing it [MINOR] Make readVersion lazy value Dec 13, 2024
@tedyu tedyu changed the title [MINOR] Make readVersion lazy value [Follow-up] [MINOR] Make readVersion lazy value Dec 13, 2024
@tedyu
Copy link
Contributor Author

tedyu commented Dec 13, 2024

@HeartSaVioR
Can you review this PR ?

Copy link
Contributor

@HeartSaVioR HeartSaVioR left a comment

Choose a reason for hiding this comment

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

Could you please add SPARK ticket label to the title as it's a follow-up from existing JIRA ticket?

@WweiL
Copy link
Contributor

WweiL commented Dec 14, 2024

I think it should be renamed as

[SPARK-50360][FOLLOWUP][MINOR] Make readVersion lazy value

@tedyu tedyu changed the title [Follow-up] [MINOR] Make readVersion lazy value [SPARK-50360] [Follow-up] [MINOR] Make readVersion lazy value Dec 14, 2024
@tedyu
Copy link
Contributor Author

tedyu commented Dec 14, 2024

@HeartSaVioR
Please take another look.
Test failure was not caused by the PR.

@HeartSaVioR HeartSaVioR changed the title [SPARK-50360] [Follow-up] [MINOR] Make readVersion lazy value [SPARK-50360][SS][FOLLOWUP][MINOR] Make readVersion lazy value Dec 16, 2024
@HeartSaVioR
Copy link
Contributor

The failure only happened with TPC, which isn't relevant.

@HeartSaVioR
Copy link
Contributor

Thanks! Merging to master.

@HeartSaVioR
Copy link
Contributor

Forgot to call out +1

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.

3 participants