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: Revert named stores in expected topologies, disable naming stores from StreamJoined, re-enable join tests. #3550

Conversation

bbejeck
Copy link
Contributor

@bbejeck bbejeck commented Oct 11, 2019

Description

This PR disables the naming of stores in join operations and reverts the named stores in the expected topologies files. Note that in the future KSQL can easily re-enable the naming of state stores for joins. After this PR is merged, we should be able to close #3364

Testing done

Updated all expected topology files and ran all tests, everything passing.

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 11, 2019 15:37
@bbejeck bbejeck changed the title fix: Revert named stores in expected topologies, disable naming stores …s from StreamJoined, re-enable join tests. fix: Revert named stores in expected topologies, disable naming stores from StreamJoined, re-enable join tests. Oct 11, 2019
Copy link
Contributor

@big-andy-coates big-andy-coates left a comment

Choose a reason for hiding this comment

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

Thanks @bbejeck - really appreciate you seeing this one through to the end!

Reverting the expected topologies ensures 5.4 goes out without a backwards incompatible change on internal topic and state store names.

Thanks!

@@ -1,8 +1,8 @@
{
"tests": [
{
"comment": "re-enable after KIP-479 is merged",
"enabled": false,
"comment": "Tests covering the use of Joins",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: would you mind just removing the comment and enabled fields from the JSON for each of these please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

@big-andy-coates big-andy-coates requested a review from a team October 11, 2019 15:50
@bbejeck
Copy link
Contributor Author

bbejeck commented Oct 11, 2019

@big-andy-coates updated this per comments

@bbejeck bbejeck requested a review from vcrfxia October 11, 2019 16:04
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.

🎉 thanks Bill!

@bbejeck bbejeck merged commit 0b8ccc1 into master Oct 11, 2019
@bbejeck
Copy link
Contributor Author

bbejeck commented Oct 11, 2019

Merged #3550 into master.

@bbejeck
Copy link
Contributor Author

bbejeck commented Oct 11, 2019

Thanks @big-andy-coates and @agavra for the reviews!

@bbejeck bbejeck deleted the fix_remove_with_store_name_method_revert_topologies_enable_join_tests branch October 11, 2019 16:55
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.

Re-enable disabled JOIN tests.
3 participants