KAFKA-5361: Add more integration tests for Streams EOS #3276

mjsax · 2017-06-08T23:59:42Z

multi-subtopology tests
fencing test
producer fenced bug fix: Streams did not recover correctly from ProducerFencedException

mjsax · 2017-06-09T00:00:30Z

Call for review @enothereska @dguy @bbejeck @guozhangwang

asfbot · 2017-06-09T00:03:53Z

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

asfbot · 2017-06-09T00:05:01Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/5073/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-06-09T01:51:32Z

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

asfbot · 2017-06-09T01:51:57Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/5076/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-06-09T19:27:27Z

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

asfbot · 2017-06-09T19:43:07Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/5092/
Test PASSed (JDK 8 and Scala 2.12).

- multi-subtopology tests - fencing test

cleanup fencing test after rebase

mjsax · 2017-06-09T21:22:26Z

Call for review @enothereska @dguy @bbejeck @guozhangwang

asfbot · 2017-06-09T22:21:49Z

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

asfbot · 2017-06-09T22:22:28Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/5102/
Test FAILed (JDK 8 and Scala 2.12).

mjsax · 2017-06-09T22:30:52Z

Test failure unrelated: kafka.security.auth.SimpleAclAuthorizerTest.testHighConcurrencyModificationOfResourceAcls

mjsax · 2017-06-09T22:30:58Z

Retest this please

asfbot · 2017-06-09T23:38:13Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/5104/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-06-09T23:39:27Z

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

guozhangwang

Left some minor comments.

Could you trigger say 5 consecutive builds on Jenkins and check their results?

guozhangwang · 2017-06-12T20:04:17Z

clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java

@@ -57,7 +56,7 @@ public static void registerAppInfo(String prefix, String id) {
            AppInfo mBean = new AppInfo();
            ManagementFactory.getPlatformMBeanServer().registerMBean(mBean, name);
        } catch (JMException e) {
-            log.warn("Error registering AppInfo mbean", e);
+            log.warn("Error registering AppInfo mbean: {}", e.getMessage());


Why this change?

I though we don't want to print stack traces for WARN level logs?

There is no such policy at the moment. Was there a specific issue with this line?

The issue we observed in Streams is, that people see a stack trace and get worried -- they don't pay attention to the log level that is WARN -- thus it seems better to never include stack traces on WARN to not confuse users. I can revert, too.

WARN is something to be worried about, right? If it's just informational, it should be INFO.

I agree. But WARN should still not contain a stack trace -- that's for ERROR IMHO.

That seems pretty arbitrary though since you just agreed that users should be worried either way. The problem may be that people are using warn when they should use info.

guozhangwang · 2017-06-12T20:06:19Z

streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamThread.java

@@ -259,7 +260,7 @@ void retryWithBackoff(final Map<TaskId, Set<TopicPartition>> tasksToBeCreated, f
                    } catch (final LockException e) {
                        // ignore and retry
                        if (!retryingTasks.contains(taskId)) {
-                            log.warn("{} Could not create task {}. Will retry. {}", logPrefix, taskId, e);
+                            log.warn("{} Could not create task {}. Will retry. {}", logPrefix, taskId, e.getMessage());


nit: could not create task {} due to {}. Will retry.

guozhangwang · 2017-06-12T20:07:14Z

streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamThread.java

@@ -272,6 +273,7 @@ void retryWithBackoff(final Map<TaskId, Set<TopicPartition>> tasksToBeCreated, f
                try {
                    Thread.sleep(backoffTimeMs);
                    backoffTimeMs <<= 1;
+                    backoffTimeMs = Math.min(backoffTimeMs, maxBackoffTimeMs);


Why this change again? Or could you add this to the description of the PR (it seems not included)?

Oh. My bad. That is unrelated and I included it to not forget about it -- currently, with exponential increase of waiting time users did report unsatisfying behavior for this. I think we need to fix it... This should also be back ported to 0.10.2... But maybe it should not be part of this PR. But not sure how else we can get it into 0.11 ;)

Opened #3312 for 0.10.2

Hmm.. I was not aware of this, and could you point me to the un-statisfying behavior that user complain about? Would like to make sure if this is really an issue to fix before merging.

For this PR, I'd suggest we do not include it so that it can be merged.

Reverted this and created #3327

mjsax · 2017-06-13T01:01:45Z

Updated.

asfbot · 2017-06-13T02:01:43Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/5198/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-06-13T02:03:49Z

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

asfbot · 2017-06-14T00:19:28Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/5250/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-06-14T00:43:53Z

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

- multi-subtopology tests - fencing test - producer fenced bug fix: Streams did not recover correctly from ProducerFencedException Author: Matthias J. Sax <matthias@confluent.io> Reviewers: Ismael Juma <ismael@juma.me.uk>, Bill Bejeck <bill@confluent.io>, Guozhang Wang <wangguoz@gmail.com> Closes #3276 from mjsax/kafka-5361-add-eos-integration-tests-for-streams-api (cherry picked from commit 160933b) Signed-off-by: Guozhang Wang <wangguoz@gmail.com>

guozhangwang · 2017-06-14T16:19:35Z

LGTM and merged to trunk / 0.11.0.

mjsax force-pushed the kafka-5361-add-eos-integration-tests-for-streams-api branch from 383937b to cf2c6da Compare June 9, 2017 18:27

mjsax added 4 commits June 9, 2017 12:51

KAFKA-5361: Add more integration tests

3a8489d

- multi-subtopology tests - fencing test

fixed chechstyle errors

5be2256

disable topic auto create to avoid TopicExistsException

f4380a2

fixed Streams bug: did not recover from ProducerFencedException

5e7fd50

cleanup fencing test after rebase

mjsax force-pushed the kafka-5361-add-eos-integration-tests-for-streams-api branch from cf2c6da to 5e7fd50 Compare June 9, 2017 21:20

bbejeck approved these changes Jun 9, 2017

View reviewed changes

guozhangwang reviewed Jun 12, 2017

View reviewed changes

Guozhang's comments

0b60cd6

mjsax added 2 commits June 13, 2017 15:43

Revert logging change

9a96f84

revert upper-bound backoff-time

b8075a5

mjsax force-pushed the kafka-5361-add-eos-integration-tests-for-streams-api branch from 22e4792 to b8075a5 Compare June 13, 2017 22:47

asfgit closed this in 160933b Jun 14, 2017

mjsax deleted the kafka-5361-add-eos-integration-tests-for-streams-api branch June 14, 2017 16:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-5361: Add more integration tests for Streams EOS #3276

KAFKA-5361: Add more integration tests for Streams EOS #3276

mjsax commented Jun 8, 2017 •

edited

Loading

mjsax commented Jun 9, 2017

asfbot commented Jun 9, 2017

asfbot commented Jun 9, 2017

asfbot commented Jun 9, 2017

asfbot commented Jun 9, 2017

asfbot commented Jun 9, 2017

asfbot commented Jun 9, 2017

mjsax commented Jun 9, 2017

asfbot commented Jun 9, 2017

asfbot commented Jun 9, 2017

mjsax commented Jun 9, 2017

mjsax commented Jun 9, 2017

asfbot commented Jun 9, 2017

asfbot commented Jun 9, 2017

guozhangwang left a comment

guozhangwang Jun 12, 2017

mjsax Jun 12, 2017

ijuma Jun 13, 2017

mjsax Jun 13, 2017

ijuma Jun 13, 2017

mjsax Jun 14, 2017

ijuma Jun 14, 2017

guozhangwang Jun 12, 2017

guozhangwang Jun 12, 2017

mjsax Jun 12, 2017

mjsax Jun 13, 2017

guozhangwang Jun 13, 2017

mjsax Jun 13, 2017

mjsax commented Jun 13, 2017

asfbot commented Jun 13, 2017

asfbot commented Jun 13, 2017

asfbot commented Jun 14, 2017

asfbot commented Jun 14, 2017

guozhangwang commented Jun 14, 2017

KAFKA-5361: Add more integration tests for Streams EOS #3276

KAFKA-5361: Add more integration tests for Streams EOS #3276

Conversation

mjsax commented Jun 8, 2017 • edited Loading

mjsax commented Jun 9, 2017

asfbot commented Jun 9, 2017

asfbot commented Jun 9, 2017

asfbot commented Jun 9, 2017

asfbot commented Jun 9, 2017

asfbot commented Jun 9, 2017

asfbot commented Jun 9, 2017

mjsax commented Jun 9, 2017

asfbot commented Jun 9, 2017

asfbot commented Jun 9, 2017

mjsax commented Jun 9, 2017

mjsax commented Jun 9, 2017

asfbot commented Jun 9, 2017

asfbot commented Jun 9, 2017

guozhangwang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjsax commented Jun 13, 2017

asfbot commented Jun 13, 2017

asfbot commented Jun 13, 2017

asfbot commented Jun 14, 2017

asfbot commented Jun 14, 2017

guozhangwang commented Jun 14, 2017

mjsax commented Jun 8, 2017 •

edited

Loading