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

KAFKA-9020: Streams sub-topologies should be sorted by sink -> source relationship #7495

Merged
merged 6 commits into from Oct 14, 2019

Conversation

ableegoldman
Copy link
Contributor

Subtopologies are currently ordered alphabetically by source node, which prior to KIP-307 happened to always result in the "correct" (ie topological) order. Now that users may name their nodes anything they want, we must explicitly order them so that upstream node groups/subtopologies come first and the downstream ones come after.

@ableegoldman
Copy link
Contributor Author

@mjsax mjsax added the streams label Oct 11, 2019
@guozhangwang
Copy link
Contributor

ping @bbejeck @vvcephei could you guys take a look?

Copy link
Contributor

@bbejeck bbejeck left a comment

Choose a reason for hiding this comment

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

Thanks for this fix @ableegoldman! LGTM

@bbejeck
Copy link
Contributor

bbejeck commented Oct 11, 2019

Java 11/2.12 failed with Task :connect:mirror:integrationTest FAILED

Java 11/2.13 failed with kafka.api.ConsumerBounceTest.testRollingBrokerRestartsWithSmallerMaxGroupSizeConfigDisruptsBigGroup

Java 8 failed with FATAL: Unable to delete script file /tmp/jenkins6761014166594532618.sh 06:53:19 java.io.EOFException

retest this please

@ableegoldman
Copy link
Contributor Author

Java 8 and Java 11.12 passed
Java 11 says this failed but when you click to see the stacktrace it says it passed, sooo...?
KTableKTableForeignKeyJoinIntegrationTest.doLeftJoinFilterOutRapidlyChangingForeignKeyValues

@bbejeck
Copy link
Contributor

bbejeck commented Oct 14, 2019

ping either of @guozhangwang @vvcephei for a second review

@bbejeck
Copy link
Contributor

bbejeck commented Oct 14, 2019

Java 11/2.13 failed - test results already cleaned up.
Java 11/2.12 and Java 8 passed

retest this please

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.

LGTM.

@bbejeck
Copy link
Contributor

bbejeck commented Oct 14, 2019

on this PR builder run Java 11/2.12 and Java 11/2.13 passed
on previous PR builder run Java 8 and Java 11/2.12 passed, so between the two all passed.
Merging this now.

@bbejeck bbejeck merged commit b006205 into apache:trunk Oct 14, 2019
@bbejeck
Copy link
Contributor

bbejeck commented Oct 14, 2019

Merged #7495 into trunk.

bbejeck pushed a commit that referenced this pull request Oct 14, 2019
… relationship (#7495)

Subtopologies are currently ordered alphabetically by source node, which prior to KIP-307 happened to always result in the "correct" (ie topological) order. Now that users may name their nodes anything they want, we must explicitly order them so that upstream node groups/subtopologies come first and the downstream ones come after.

Reviewers: Guozhang Wang <wangguoz@gmail.com>, Bill Bejeck <bbejeck@gmail.com>
@bbejeck
Copy link
Contributor

bbejeck commented Oct 14, 2019

cherry-picked to 2.4


for (final String nodeName : Utils.sorted(allSourceNodes)) {
// Traverse in topological order
for (final String nodeName : nodeFactories.keySet()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

q: how could we guarantee a topological traversal on a map without order?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nodeFactories is in topological order, for example in InternalStreamsBuilder#buildAndOptimizeTopology we only add a node once all its parents have been written to the topology already

@ableegoldman ableegoldman deleted the subtopology-order branch June 26, 2020 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants