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: changes required for compatibility with KIP-479 #3466

Merged
merged 5 commits into from
Oct 9, 2019

Conversation

bbejeck
Copy link
Contributor

@bbejeck bbejeck commented Oct 3, 2019

Description

This PR introduces the StreamJoined object from KIP-479. Specifically this PR

  1. Removes use of the now deprecated KStream#join(KStream,....Joined) methods in favor of KStream#join(KStream,....StreamJoined)
  2. Keeps the name of the stores so none of the expected topologies needed changing. This was a judgment call on my part, and the KSQL team will need to let me know their preferences.

Testing done

I updated existing tests. Since this PR does not introduce new behavior, I felt that updating the current tests is sufficient.
EDIT: I ran the QueryTranslationTest, ksql-engine, and ksql-streams tests locally and all passed.

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@bbejeck bbejeck requested a review from a team as a code owner October 3, 2019 13:52
final String name,
final String storeName) {
return StreamJoined.with(keySerde, leftSerde, rightSerde)
.withName(name).withStoreName(storeName);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The withStoreName is the key change/addition. If we want to stick with generated store names, we can omit the withStoreName call and update the expected topologies for the QueryTranslationTest.

Copy link
Contributor

@agavra agavra 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 @bbejeck

@agavra agavra requested a review from a team October 3, 2019 18:48
Copy link
Contributor

@rodesai rodesai left a comment

Choose a reason for hiding this comment

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

The code changes to ksql-streams look good. You won't need the changes to SchemaKStream after you rebase.

@bbejeck bbejeck force-pushed the fix_update_for_new_stream_joined_config_object branch 2 times, most recently from 530ae49 to 689fc83 Compare October 4, 2019 00:58
@bbejeck
Copy link
Contributor Author

bbejeck commented Oct 4, 2019

rebased and updated this PR

@bbejeck bbejeck force-pushed the fix_update_for_new_stream_joined_config_object branch from 689fc83 to 1a87af9 Compare October 9, 2019 03:32
@bbejeck
Copy link
Contributor Author

bbejeck commented Oct 9, 2019

rebased this

@@ -251,6 +254,7 @@ public void shouldDoInnerJoin() {
}

@Test
@SuppressFBWarnings
Copy link
Contributor Author

@bbejeck bbejeck Oct 9, 2019

Choose a reason for hiding this comment

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

Spotbugs doesn't like line 266, adding this suppression for now

Return value of StreamJoinedFactory.create(Serde, Serde, Serde, String, String) ignored, but method has no side effect [io.confluent.ksql.execution.streams.StreamStreamJoinBuilderTest] At StreamStreamJoinBuilderTest.java:[line 264] RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT

@bbejeck bbejeck changed the title fix: changes required for compatibility with KIP-479[Don't Merge Yet] fix: changes required for compatibility with KIP-479 Oct 9, 2019
@bbejeck bbejeck merged commit 567f056 into master Oct 9, 2019
@bbejeck
Copy link
Contributor Author

bbejeck commented Oct 9, 2019

Merged #3466 into master.

@bbejeck bbejeck deleted the fix_update_for_new_stream_joined_config_object branch October 9, 2019 15:52
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.

3 participants