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-5366: Add concurrent reads to transactions system test #3217

Conversation

apurvam
Copy link
Contributor

@apurvam apurvam commented Jun 2, 2017

This currently fails in multiple ways. One of which is most likely KAFKA-5355, where the concurrent consumer reads duplicates.

During broker bounces, the concurrent consumer misses messages completely. This is another bug.

@apurvam apurvam force-pushed the KAFKA-5366-add-concurrent-reads-to-transactions-system-test branch from 71fcad1 to 051e77a Compare June 2, 2017 22:38
@asfbot
Copy link

asfbot commented Jun 3, 2017

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

@asfbot
Copy link

asfbot commented Jun 3, 2017

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

@apurvam apurvam force-pushed the KAFKA-5366-add-concurrent-reads-to-transactions-system-test branch from 051e77a to f6f15cb Compare June 6, 2017 00:51
@asfbot
Copy link

asfbot commented Jun 6, 2017

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

@asfbot
Copy link

asfbot commented Jun 6, 2017

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

@apurvam apurvam force-pushed the KAFKA-5366-add-concurrent-reads-to-transactions-system-test branch from f6f15cb to 52028b8 Compare June 6, 2017 04:49
@asfbot
Copy link

asfbot commented Jun 6, 2017

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

@asfbot
Copy link

asfbot commented Jun 6, 2017

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

@apurvam apurvam force-pushed the KAFKA-5366-add-concurrent-reads-to-transactions-system-test branch from 36603a1 to 07b378f Compare June 6, 2017 18:24
Copy link

@hachikuji hachikuji 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 the patch. Left a couple trivial comments.

@@ -242,6 +242,9 @@ public synchronized boolean ensurePartitionAdded(TopicPartition tp) {
// We should enter this branch in an error state because if this partition is already in the transaction,
// there is a chance that the corresponding batch is in retry. So we must let it completely flush.
if (!(partitionsInTransaction.contains(tp) || isPartitionPending(tp))) {
log.debug("partitionsInTransaction: [{}], newPartitionsInTransaction: [{}], " +

Choose a reason for hiding this comment

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

Is this intentional or were you just using this for your own debugging? If it's intentional, we should probably add the log prefix and make the message a little clearer.

# ensure that the consumer is up.
wait_until(lambda: (len(consumer.messages_consumed[1]) > 0) == True,
timeout_sec=60,
err_msg="Consumer failed to consume any messages for for %ds" %\

Choose a reason for hiding this comment

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

nit: extra "for"

return consumer

def drain_consumer(self, consumer):
# wait until the consumer closes, which will be 15 seconds after

Choose a reason for hiding this comment

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

This comment doesn't seem to match what the wait_until is doing.

@asfbot
Copy link

asfbot commented Jun 6, 2017

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

@asfbot
Copy link

asfbot commented Jun 6, 2017

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

@apurvam
Copy link
Contributor Author

apurvam commented Jun 6, 2017

These are starting to stabilize:

================================================================================
SESSION REPORT (ALL TESTS)
ducktape version: 0.6.0
session_id:       2017-06-06--010
run time:         13 minutes 10.752 seconds
tests run:        4
passed:           4
failed:           0
ignored:          0
================================================================================
test_id:    kafkatest.tests.core.transactions_test.TransactionsTest.test_transactions.failure_mode=clean_bounce.bounce_target=brokers
status:     PASS
run time:   4 minutes 7.220 seconds
--------------------------------------------------------------------------------
test_id:    kafkatest.tests.core.transactions_test.TransactionsTest.test_transactions.failure_mode=clean_bounce.bounce_target=clients
status:     PASS
run time:   2 minutes 17.809 seconds
--------------------------------------------------------------------------------
test_id:    kafkatest.tests.core.transactions_test.TransactionsTest.test_transactions.failure_mode=hard_bounce.bounce_target=brokers
status:     PASS
run time:   3 minutes 2.829 seconds
--------------------------------------------------------------------------------
test_id:    kafkatest.tests.core.transactions_test.TransactionsTest.test_transactions.failure_mode=hard_bounce.bounce_target=clients
status:     PASS
run time:   3 minutes 42.069 seconds
--------------------------------------------------------------------------------

@apurvam
Copy link
Contributor Author

apurvam commented Jun 6, 2017

Kicked off a branch builder for this single test. Not sure if jenkins is ready though:

http://jenkins.confluent.io/job/system-test-kafka-branch-builder/902/

@asfbot
Copy link

asfbot commented Jun 6, 2017

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

Copy link

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

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

LGTM. The coordinator disconnect handling is a good find.

@asfbot
Copy link

asfbot commented Jun 6, 2017

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

@apurvam
Copy link
Contributor Author

apurvam commented Jun 6, 2017

@asfbot
Copy link

asfbot commented Jun 6, 2017

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

@apurvam
Copy link
Contributor Author

apurvam commented Jun 7, 2017

@hachikuji
Copy link

Merging to trunk and 0.11.0.

@asfgit asfgit closed this in 202cb8e Jun 7, 2017
asfgit pushed a commit that referenced this pull request Jun 7, 2017
This currently fails in multiple ways. One of which is most likely KAFKA-5355, where the concurrent consumer reads duplicates.

During broker bounces, the concurrent consumer misses messages completely. This is another bug.

Author: Apurva Mehta <apurva@confluent.io>

Reviewers: Jason Gustafson <jason@confluent.io>

Closes #3217 from apurvam/KAFKA-5366-add-concurrent-reads-to-transactions-system-test

(cherry picked from commit 202cb8e)
Signed-off-by: Jason Gustafson <jason@confluent.io>
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.

3 participants