MINOR: Remove redundant SuppressIntegrationTests by vvcephei · Pull Request #5896 · apache/kafka

vvcephei · 2018-11-08T23:16:11Z

The removed tests have counterparts covered by SuppressScenarioTest using the TopologyTestDriver.

This will speed up the build and improve stability in the CPU-constrained Jenkins environment.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

vvcephei · 2018-11-08T23:22:54Z

@mjsax @bbejeck Please review when you have the chance.

vvcephei · 2018-11-08T23:23:44Z

We will run the tests 10 times and verify no SuppressIntegrationTest failures before merging.

vvcephei · 2018-11-08T23:29:01Z

Java 8:
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/17612/
Cancelled by next commit

Java 11:
https://builds.apache.org/job/kafka-pr-jdk11-scala2.12/552/
Failed:
kafka.admin.AclCommandTest.classMethod
kafka.admin.AclCommandTest.classMethod
kafka.admin.ResetConsumerGroupOffsetTest.testResetOffsetsNotExistingGroup
kafka.api.AuthorizerIntegrationTest.testTransactionalProducerTopicAuthorizationExceptionInCommit
kafka.api.CustomQuotaCallbackTest.testCustomQuotaCallback
kafka.api.GroupEndToEndAuthorizationTest.testTwoConsumersWithDifferentSaslCredentials
kafka.api.GroupEndToEndAuthorizationTest.classMethod
kafka.api.PlaintextConsumerTest.testLowMaxFetchSizeForRequestAndPartition
kafka.api.PlaintextConsumerTest.testCoordinatorFailover
kafka.api.SaslPlainPlaintextConsumerTest.testCoordinatorFailover
kafka.api.SaslPlaintextConsumerTest.testCoordinatorFailover
kafka.api.SaslSslAdminClientIntegrationTest.testAclDescribe
kafka.api.SaslSslAdminClientIntegrationTest.testLegacyAclOpsNeverAffectOrReturnPrefixed
kafka.api.SaslSslAdminClientIntegrationTest.testAttemptToCreateInvalidAcls
kafka.server.CreateTopicsRequestWithPolicyTest.classMethod
kafka.server.CreateTopicsRequestWithPolicyTest.classMethod
kafka.server.MultipleListenersWithDefaultJaasContextTest.classMethod
kafka.server.MultipleListenersWithDefaultJaasContextTest.classMethod
kafka.tools.MirrorMakerIntegrationTest.testCommaSeparatedRegex
org.apache.kafka.streams.integration.RepartitionOptimizingIntegrationTest.shouldSendCorrectRecords_OPTIMIZED
org.apache.kafka.streams.integration.RepartitionOptimizingIntegrationTest.shouldSendCorrectResults_NO_OPTIMIZATION
org.apache.kafka.streams.integration.RepartitionWithMergeOptimizingIntegrationTest.shouldSendCorrectRecords_OPTIMIZED
org.apache.kafka.streams.integration.ResetIntegrationTest.shouldNotAllowToResetWhenInputTopicAbsent
org.apache.kafka.streams.integration.ResetIntegrationWithSslTest.testReprocessingFromScratchAfterResetWithIntermediateUserTopic
org.apache.kafka.streams.integration.SuppressionIntegrationTest.shouldShutdownWhenRecordConstraintIsViolated

…terparts ping

bbejeck

LGTM, but I think we still need some tests with a live broker especially when suppression with spill to disk is implemented. Given what you said in the PR comments, maybe we can include a system test?

mjsax

Would be good to get @guozhangwang thoughts on this, too.

vvcephei · 2018-11-09T00:35:41Z

hah. Looks like I only needed one run to fail my own target: https://builds.apache.org/job/kafka-pr-jdk11-scala2.12/552/testReport/junit/org.apache.kafka.streams.integration/SuppressionIntegrationTest/shouldShutdownWhenRecordConstraintIsViolated/

vvcephei · 2018-11-09T00:40:52Z

@bbejeck I share your concern, and I guess it's an important thing to hash out up front...
This PR is essentially switching test coverage from Integration tests to TopologyTestDriver tests. Doing so trades covering some failure modes for increased test speed and (presumably) decreased flakiness.

Do you think this tradeoff is worth it? In particular, are we giving up meaningful test coverage and losing the value of testing if we make changes like this?

vvcephei · 2018-11-09T00:46:47Z

Java8:
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/17615/

Java11:
https://builds.apache.org/job/kafka-pr-jdk11-scala2.12/555/

vvcephei · 2018-11-09T00:49:50Z

The single remaining SuppressionIntegrationTest still timed out after 30s, so I'm also experimenting with bumping the timeout to 60s.

vvcephei · 2018-11-09T16:29:07Z

no java 8 fails.
java 11 fails:
kafka.api.PlaintextConsumerTest.testLowMaxFetchSizeForRequestAndPartition
kafka.api.SaslPlainPlaintextConsumerTest.testCoordinatorFailover
kafka.api.SaslSslAdminClientIntegrationTest.testAclDescribe
kafka.api.SaslSslAdminClientIntegrationTest.testLegacyAclOpsNeverAffectOrReturnPrefixed
kafka.server.epoch.EpochDrivenReplicationProtocolAcceptanceTest.shouldSurviveFastLeaderChange
kafka.admin.ResetConsumerGroupOffsetTest.testResetOffsetsNotExistingGroup
kafka.api.CustomQuotaCallbackTest.testCustomQuotaCallback
kafka.tools.MirrorMakerIntegrationTest.testCommaSeparatedRegex
org.apache.kafka.streams.integration.ResetIntegrationTest.shouldNotAllowToResetWhenInputTopicAbsent
org.apache.kafka.streams.integration.ResetIntegrationWithSslTest.testReprocessingFromScratchAfterResetWithIntermediateUserTopic

retest this, please.

vvcephei · 2018-11-09T22:38:45Z

no java 8 fails.

java 11 fails:
https://builds.apache.org/job/kafka-pr-jdk11-scala2.12/567/

kafka.admin.ResetConsumerGroupOffsetTest.testResetOffsetsNotExistingGroup
kafka.api.CustomQuotaCallbackTest.testCustomQuotaCallback
kafka.api.PlaintextConsumerTest.testLowMaxFetchSizeForRequestAndPartition
kafka.api.PlaintextConsumerTest.testCoordinatorFailover
kafka.api.SaslSslAdminClientIntegrationTest.testAclDescribe
kafka.api.SaslSslAdminClientIntegrationTest.testLegacyAclOpsNeverAffectOrReturnPrefixed
kafka.server.ListOffsetsRequestTest.testResponseIncludesLeaderEpoch
org.apache.kafka.streams.integration.RepartitionWithMergeOptimizingIntegrationTest.shouldSendCorrectRecords_OPTIMIZED
org.apache.kafka.streams.integration.ResetIntegrationTest.shouldNotAllowToResetWhenInputTopicAbsent
org.apache.kafka.streams.integration.ResetIntegrationWithSslTest.testReprocessingFromScratchAfterResetWithIntermediateUserTopic
kafka.api.SaslPlainPlaintextConsumerTest.testCoordinatorFailover
kafka.tools.MirrorMakerIntegrationTest.testCommaSeparatedRegex

vvcephei · 2018-11-09T22:38:51Z

retest this, please

mjsax · 2018-11-10T00:15:19Z

I personally don't believe that the risk for this particular test is large. It's testing with a single partition and everything is based on event-time.

The only difference I see is, that using TopologyTestDriver we flush the KTable cache after each records. But this should not impact the test.

Btw: I am wondering, why we don't remove the whole test? The two remaining error scenarios can be tested as unit-test, too, IMHO.

vvcephei · 2018-11-10T00:38:46Z

I wanted to actually test that Streams shuts down, since that's what the feature is advertised to do. My reasoning was that I could have a unit test that verifies it throws an exception, but the only way to make sure that Streams shuts down is actually to run the whole thing.

Does this seem like good thinking, or am I overengineering?

vvcephei · 2018-11-10T00:39:41Z

Java 11 fails:
https://builds.apache.org/job/kafka-pr-jdk11-scala2.12/575/

kafka.api.PlaintextConsumerTest.testLowMaxFetchSizeForRequestAndPartition
kafka.api.PlaintextConsumerTest.testCoordinatorFailover
kafka.api.SaslPlainPlaintextConsumerTest.testCoordinatorFailover
kafka.api.SaslSslAdminClientIntegrationTest.testAclAuthorizationDenied
kafka.api.SaslSslAdminClientIntegrationTest.testAclOperations
kafka.api.SaslSslAdminClientIntegrationTest.testInvalidAlterConfigs
kafka.api.SaslSslAdminClientIntegrationTest.testAlterLogDirsAfterDeleteRecords
kafka.api.SaslSslAdminClientIntegrationTest.testConsumeAfterDeleteRecords
kafka.api.SaslSslAdminClientIntegrationTest.testReplicaCanFetchFromLogStartOffsetAfterDeleteRecords
kafka.api.SslConsumerTest.testCoordinatorFailover
kafka.admin.ResetConsumerGroupOffsetTest.testResetOffsetsNotExistingGroup
kafka.api.CustomQuotaCallbackTest.testCustomQuotaCallback
kafka.api.SaslSslAdminClientIntegrationTest.testAclDescribe
kafka.api.SaslSslAdminClientIntegrationTest.testLegacyAclOpsNeverAffectOrReturnPrefixed
kafka.api.SaslSslConsumerTest.testCoordinatorFailover
kafka.api.TransactionsTest.testFencingOnTransactionExpiration
kafka.tools.MirrorMakerIntegrationTest.testCommaSeparatedRegex
org.apache.kafka.streams.integration.RepartitionWithMergeOptimizingIntegrationTest.shouldSendCorrectRecords_OPTIMIZED
org.apache.kafka.streams.integration.ResetIntegrationTest.shouldNotAllowToResetWhenInputTopicAbsent
org.apache.kafka.streams.integration.ResetIntegrationWithSslTest.testReprocessingFromScratchAfterResetWithIntermediateUserTopic

retest this, please.

mjsax · 2018-11-10T01:02:13Z

Does this seem like good thinking, or am I overengineering?

Depends if we have a test KafkaStreamsTest#shouldShutDownOnUncaughtException or similar -- if yes, and if you believe in decomposition, it should be sufficient to test that an exception is thrown.

Note: if we don't have this case, it might be worth to add this test to get rid of the other two. I personally believe in decomposition, and both tests tests from this class have a huge overlap (testing the shutdown part) that the new test could extract and cover at once.

vvcephei · 2018-11-13T17:21:16Z

Since opening this ticket, the Java11 version has been upgraded (https://issues.apache.org/jira/browse/INFRA-17235), and an issue with our build configuration has also been fixed, so this PR may not be needed to improve stability.

I'll put the question to the reviewers whether we should still proceed.

Retest this, please.

vvcephei · 2018-11-15T00:42:28Z

That is an unrelated failure:
kafka.admin.ReassignPartitionsClusterTest.shouldExecuteThrottledReassignment

and it's the same for java 8 and 11.

retest this, please.

The removed tests have counterparts covered by SuppressScenarioTest using the TopologyTestDriver. This will speed up the build and improve stability in the CPU-constrained Jenkins environment. Reviewers: Bill Bejeck <bill@confluent.io>, Matthias J. Sax <matthias@confluent.io>, Guozhang Wang <wangguoz@gmail.com>

vvcephei changed the title ~~WIP: testing a bunch of Streams test changes to try and improve Java11 stability~~ MINOR: Remove redundant SuppressIntegrationTests Nov 8, 2018

Remove Suppress Integration tests that have SuppressScenarioTest coun…

2c35f43

…terparts ping

bbejeck approved these changes Nov 8, 2018

View reviewed changes

mjsax approved these changes Nov 8, 2018

View reviewed changes

increase timeout

bb722b1

guozhangwang approved these changes Nov 15, 2018

View reviewed changes

guozhangwang merged commit abc0959 into apache:trunk Nov 15, 2018

vvcephei deleted the test-resilience branch November 15, 2018 22:15

Conversation

vvcephei commented Nov 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

vvcephei commented Nov 8, 2018

Uh oh!

vvcephei commented Nov 8, 2018

Uh oh!

vvcephei commented Nov 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

mjsax left a comment

Choose a reason for hiding this comment

Uh oh!

vvcephei commented Nov 9, 2018

Uh oh!

vvcephei commented Nov 9, 2018

Uh oh!

vvcephei commented Nov 9, 2018

Uh oh!

vvcephei commented Nov 9, 2018

Uh oh!

vvcephei commented Nov 9, 2018

Uh oh!

vvcephei commented Nov 9, 2018

Uh oh!

vvcephei commented Nov 9, 2018

Uh oh!

mjsax commented Nov 10, 2018

Uh oh!

vvcephei commented Nov 10, 2018

Uh oh!

vvcephei commented Nov 10, 2018

Uh oh!

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

Uh oh!

Uh oh!

vvcephei commented Nov 13, 2018

Uh oh!

vvcephei commented Nov 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vvcephei commented Nov 8, 2018 •

edited

Loading

vvcephei commented Nov 8, 2018 •

edited

Loading

mjsax commented Nov 10, 2018 •

edited

Loading