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

fix: remove usages of internal Streams API/variable that was removed upstream #8660

Conversation

ableegoldman
Copy link
Contributor

@ableegoldman ableegoldman commented Jan 26, 2022

Apparently we used the StreamsMetadataImpl.NOT_AVAILABLE variable throughout the ksql repo, but we very much should not have been since this is actually not in the public API of Streams. The risk of using internal APIs is that they have no public contract or compatability guarantees of any kind, and can for example be removed at any time without deprecation/warning.

Unfortunately this is exactly what just happened to StreamsMetadataImpl.NOT_AVAILABLE, but at least we can easily address this since it was pretty much just a convenience to begin with used for comparisons, and we no longer return any metadata for unavailable clients rather than build up this metadata and then just filter it out on the user side (ie ksql)

…StreamsMetadataImpl.NOT_AVAILABLE variable which was just deleted upstream in AK
@ableegoldman ableegoldman requested a review from a team as a code owner January 26, 2022 11:14
Copy link
Contributor

@patrickstuedi patrickstuedi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @ableegoldman !

@ableegoldman ableegoldman merged commit 9615a02 into confluentinc:master Jan 26, 2022
@ableegoldman
Copy link
Contributor Author

Merging ASAP since this is blocking the 0.24 release and the build in general, I did test locally to make sure the patch fixes the build but if there are test failures somehow that come up later they should be addressed in a followup PR

Copy link
Contributor

@guozhangwang guozhangwang left a comment

Choose a reason for hiding this comment

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

Just curious do we have a separate PR to fix the tests now?

@jnh5y
Copy link
Member

jnh5y commented Jan 26, 2022

@ableegoldman @guozhangwang I peeked at this and tossed up a PR to fix the AllHostsLocatorTest here: #8664

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants