KAFKA-6054: Fix upgrade path from Kafka Streams v0.10.0 by mjsax · Pull Request #4758 · apache/kafka

mjsax · 2018-03-22T18:07:50Z

No description provided.

mjsax · 2018-03-22T18:09:49Z

This is a port of #4746 for 0.10.2 branch plus one more commit to extend the system test accordingly. Will to more PRs for all other branches similar to this one. Please comment on #4746 first, and I'll address those comments on all PRs. The existing comments on $4746 are already included in this PR.

mjsax · 2018-03-22T18:10:25Z

Upgrade system test passed: https://jenkins.confluent.io/job/system-test-kafka-branch-builder/1563/

Triggered all Streams system tests here: https://jenkins.confluent.io/job/system-test-kafka-branch-builder/1564/

asfgit · 2018-03-22T18:14:28Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/3786/
Test FAILed (JDK 7 and Scala 2.10).

asfgit · 2018-03-22T20:27:57Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/3787/
Test FAILed (JDK 7 and Scala 2.10).

asfgit · 2018-03-22T23:10:52Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/3788/
Test FAILed (JDK 7 and Scala 2.10).

asfgit · 2018-03-23T02:41:52Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/3789/
Test FAILed (JDK 7 and Scala 2.10).

asfgit · 2018-03-23T05:31:02Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/3790/
Test FAILed (JDK 7 and Scala 2.10).

guozhangwang · 2018-03-23T05:46:36Z

This is a bit verbose as we just mentioned it earlier.

guozhangwang · 2018-03-23T05:58:31Z

Not clear what does this comment mean?

We have only access to 0.10.1.1 (official release) here but not to 0.10.1 (ie, Github branch that contains the fix for KIP-268) -- thus, we cannot test upgrade from 0.10.0 to 0.10.1 here.

Okay, the misleading part is because you mentioned broker_version, but I guess you meant new_version?

guozhangwang · 2018-03-23T06:00:37Z

For this upgrade, we do not need two rolling bounces with upgrade.from config I think?

Yes. The test does only do one rolling bounce. Not sure what you mean? Is the comment confusing? It does actually no say anything about the number of rolling bounces.

My bad, I'd better leave this comment on line 57 below. I meant that we do not need to specify 0.10.0 for the upgrade.from parameter, but instead we should be able to just write

self.do_rolling_bounce(p, "", new_version, counter)

Is that right?

Good catch!

guozhangwang · 2018-03-23T06:01:54Z

This comment seems misplaced.

guozhangwang · 2018-03-23T06:04:26Z

Hmm.. this makes me thinking, that are we effectively adding one more step on release process, that the release manager need to modify this value?

That is correct. Thinking about it, I am wondering if we could get the value from gradle.properties file?

I do not know how to do that.. or if it is even possible. I think adding this extra step is not too bad as in release.py we already wrap some other places where we need to remove -SNAPSHOT anyways, this is just for confirming, that in trunk maybe we'd update release.py as well.

mjsax · 2018-03-23T07:18:34Z

Note: Java 9 does not work on old 0.10.2 branch -- Java9 was set up later and we can ignore failing Java9 builds.

asfgit · 2018-03-23T08:27:57Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/3791/
Test PASSed (JDK 7 and Scala 2.10).

guozhangwang

Left some follow-up comments. Overall It LGTM. Please feel free to merge after addressing them.

guozhangwang · 2018-03-23T16:24:40Z

Okay, the misleading part is because you mentioned broker_version, but I guess you meant new_version?

guozhangwang · 2018-03-23T16:26:34Z

My bad, I'd better leave this comment on line 57 below. I meant that we do not need to specify 0.10.0 for the upgrade.from parameter, but instead we should be able to just write

self.do_rolling_bounce(p, "", new_version, counter)

Is that right?

guozhangwang · 2018-03-23T16:28:34Z

I do not know how to do that.. or if it is even possible. I think adding this extra step is not too bad as in release.py we already wrap some other places where we need to remove -SNAPSHOT anyways, this is just for confirming, that in trunk maybe we'd update release.py as well.

mjsax · 2018-03-23T22:14:18Z

Updated this with 0.10.1 comments and follow ups.

System tests passed: https://jenkins.confluent.io/job/system-test-kafka-branch-builder/1589/

asfgit · 2018-03-23T23:14:27Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/3792/
Test PASSed (JDK 7 and Scala 2.10).

mjsax · 2018-03-25T06:43:09Z

Retest this please

asfgit · 2018-03-25T07:33:05Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/3793/
Test PASSed (JDK 7 and Scala 2.10).

John's review Guozhang's follow up

asfgit · 2018-03-26T01:13:13Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/3794/
Test PASSed (JDK 7 and Scala 2.10).

bbejeck

Thanks @mjsax, LGTM.

mjsax · 2018-03-26T23:29:39Z

Merging this to 0.10.2 now.

vvcephei

Ah! you win. Well, here's my thoughts anyway ;)

vvcephei · 2018-03-26T23:54:26Z

        final Properties streamsConfiguration = new Properties();
        streamsConfiguration.put(StreamsConfig.APPLICATION_ID_CONFIG, "fanout-integration-test");
        streamsConfiguration.put(StreamsConfig.BOOTSTRAP_SERVERS_CONFIG, CLUSTER.bootstrapServers());
+        streamsConfiguration.setProperty(StreamsConfig.STATE_DIR_CONFIG, TestUtils.tempDirectory().getPath());


getAbsolutePath?

(as in https://github.com/apache/kafka/pull/4746/files#diff-13c6128851d974bddf5bb1132b1208f4R120)

vvcephei · 2018-03-26T23:57:02Z

        });

-        data.process(SmokeTestUtil.printProcessorSupplier("data"));
+        data.process(SmokeTestUtil.<String, Integer>printProcessorSupplier("data"));


can these be data.process(SmokeTestUtil.<>printProcessorSupplier("data")) ?

Just double checked. data.process(SmokeTestUtil.printProcessorSupplier("data")) compiles

vvcephei · 2018-03-27T00:07:07Z

            allData.put(data[i].key, new HashSet<Integer>());
        }
-        Random rand = new Random();
+        final Random rand = new Random();


Admittedly, this is a random time to bring this up, but a pattern that I like for this is:

final long seed = providedSeed != null ? providedSeed : new Random().nextLong(); System.out.println("Seed is: " + seed); final Random rand = new Random(seed);

where providedSeed comes from an env variable, command-line arg, whatever...

This way you get the same default behavior (pseudorandomly generated sequence of test values), but you also have the option to deterministically reproduce a previous pseudorandom sequence from a prior run. This can be helpful in diagnosing flaky tests.

Not saying this is needed here; I just wanted to share the pattern.

Basically I agree. So far, we did not have any issues with Random introducing flakiness that we cannot reproduce. However, I am not sure if seed = new Random().nextLong() is a good seed -- isn't this also deterministic? I am used to seed = System.currentTimeMillis() ?

mjsax · 2018-03-27T04:46:35Z

Merged to 0.10.2.

Follow up PR to address missed comments: #4778

mjsax added 2 commits March 21, 2018 22:46

KAFKA-6054: Fix upgrade path from Kafka Streams v0.10.0

40c792d

Extend system tests

1fb48e3

mjsax added the streams label Mar 22, 2018

mjsax requested review from dguy and guozhangwang March 22, 2018 18:07

Fix checkstyle

f90a523

Github comments

ebc2506

mjsax mentioned this pull request Mar 22, 2018

MINOR: Rolling bounce upgrade fixed broker system test #4690

Merged

3 tasks

mjsax mentioned this pull request Mar 23, 2018

KAFKA-6054: Fix upgrade path from Kafka Streams v0.10.0 #4761

Merged

guozhangwang reviewed Mar 23, 2018

View reviewed changes

guozhangwang approved these changes Mar 23, 2018

View reviewed changes

Update upgrade docs

4dd7828

mjsax mentioned this pull request Mar 23, 2018

KAFKA-6054: Fix upgrade path from Kafka Streams v0.10.0 #4768

Merged

Github comments

1cb064a

John's review Guozhang's follow up

mjsax force-pushed the kafka-6054-fix-upgrade-0102 branch from 8fbf6ee to 1cb064a Compare March 26, 2018 00:23

mjsax mentioned this pull request Mar 26, 2018

KAFKA-6054: Fix upgrade path from Kafka Streams v0.10.0 #4773

Merged

bbejeck approved these changes Mar 26, 2018

View reviewed changes

mjsax merged commit 67b0a94 into apache:0.10.2 Mar 26, 2018

mjsax deleted the kafka-6054-fix-upgrade-0102 branch March 26, 2018 23:31

vvcephei reviewed Mar 27, 2018

View reviewed changes

This was referenced Mar 27, 2018

MINOR: Fix typos in docs and address some missed review comments #4778

Merged

KAFKA-6054: Fix upgrade path from Kafka Streams v0.10.0 #4779

Merged

Conversation

mjsax commented Mar 22, 2018

Uh oh!

mjsax commented Mar 22, 2018

Uh oh!

mjsax commented Mar 22, 2018

Uh oh!

asfgit commented Mar 22, 2018

Uh oh!

asfgit commented Mar 22, 2018

Uh oh!

asfgit commented Mar 22, 2018

Uh oh!

asfgit commented Mar 23, 2018

Uh oh!

asfgit commented Mar 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjsax Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjsax commented Mar 23, 2018

Uh oh!

asfgit commented Mar 23, 2018

Uh oh!

guozhangwang left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjsax commented Mar 23, 2018

Uh oh!

asfgit commented Mar 23, 2018

Uh oh!

mjsax commented Mar 25, 2018

Uh oh!

asfgit commented Mar 25, 2018

Uh oh!

asfgit commented Mar 26, 2018

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

mjsax commented Mar 26, 2018

Uh oh!

vvcephei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mjsax Mar 23, 2018 •

edited

Loading

guozhangwang left a comment •

edited

Loading

mjsax Mar 27, 2018 •

edited

Loading