MINOR: Add select changes from 3rd KIP-307 PR for incrementing name index counter#6754
Conversation
|
ping @guozhangwang @mjsax @vvcephei @ableegoldman @cadonna @abbccdda for review \cc @fhussonnois |
| expected + "-table-source", | ||
| "KSTREAM-SOURCE-0000000002", | ||
| "KTABLE-SOURCE-0000000003"); | ||
| "KSTREAM-SOURCE-0000000004", |
There was a problem hiding this comment.
These number changes are valid as 2 store names (internal, not visible for IQ) are generated. I validated the topology description against a description generated by 2.2
There was a problem hiding this comment.
I see. So the diff we're seeing is against tests that were previously modified (incorrectly) to match the non-incrementing behavior that you're fixing right now?
There was a problem hiding this comment.
perfect. thanks for the reassurance!
There was a problem hiding this comment.
Actually, those test are newly added in a previous KIP-307 PR... But if Bill check agains 2.2, it should be fine.
| builder.build(); | ||
| final ProcessorTopology topology = builder.internalTopologyBuilder.rewriteTopology(new StreamsConfig(props)).build(); | ||
| assertSpecifiedNameForOperation(topology, expected, "KSTREAM-SOURCE-0000000000"); | ||
| assertSpecifiedNameForOperation(topology, expected, "KSTREAM-SOURCE-0000000001"); |
There was a problem hiding this comment.
First source node would be KSTREAM-SOURCE-0000000000 so this is a valid change in the test
| builder.build(); | ||
| final ProcessorTopology topology = builder.internalTopologyBuilder.rewriteTopology(new StreamsConfig(props)).build(); | ||
| assertSpecifiedNameForOperation(topology, "KSTREAM-SOURCE-0000000000", expected, "KSTREAM-SINK-0000000001"); | ||
| assertSpecifiedNameForOperation(topology, "KSTREAM-SOURCE-0000000000", expected, "KSTREAM-SINK-0000000002"); |
There was a problem hiding this comment.
The first source is KSTREAM-SOURCE-0000000000 second would be ...01 so this change is correct.
|
Hey @bbejeck , thanks for fixing this! I found the logic a little hard to follow, so I pulled it down to play around with it. I think that the original decision to use the Optional FP logic instead of branches, and also making the provider nullable (which was only in support of one test :/ ), was what was making it so convoluted. What do you think about this: bbejeck#1 ? |
Proposal: flatten name generation logic for readability
|
Still LGTM |
|
Merged #6754 into trunk. |
…ndex counter (apache#6754) When users provide a name for operation via the Streams DSL, we need to increment the counter used for auto-generated names to make sure any operators downstream of a named operator still produce a compatible name. This PR is a subset of apache#6411 by @fhussonnois. We need to merge this PR now because it covers cases when users name repartition topics or state stores. Updated tests to reflect the counter produces expected number even when the user provides a name. Matthias J. Sax <mjsax@apache.org>, John Roesler <john@confluent.io>
3 participants
When users provide a name for operation via the Streams DSL, we need to increment the counter used for auto-generated names to make sure any operators downstream of a named operator still produce a compatible name.
This PR is a subset of #6411 by @fhussonnois. We need to merge this PR now because it covers cases when users name repartition topics or state stores.
Updated tests to reflect the counter produces expected number even when the user provides a name.
Committer Checklist (excluded from commit message)