[GitHub] [kafka] ning2008wisc edited a comment on pull request #9224: KAFKA-10304: refactor MM2 integration tests

[GitBox <gi...@apache.org>]

ning2008wisc edited a comment on pull request #9224:
URL: https://github.com/apache/kafka/pull/9224#issuecomment-714955565


   Thanks @mimaison for your high-level advice and detailed review.
   
   (1) I responded to your every comments. A "thumb-up" means I made the suggested change
   (2) regarding the increased complexity, here is my analysis for each test case
        (a) `testReplication`: This is an existing test and no new stuff are added, except checking if topic config are also mirrored. If we believe the topic config validation is trivial, I am more than happy to remove it
        (b) `testReplicationWithEmptyPartition`: This is an existing test and no new stuff are added
        (c) `testOneWayReplicationWithAutoOffsetSync`: This is an existing test and no new stuff are added
        (d) `testWithBrokerRestart`: This is a **new** test, which introduced background producer and consumer, called `ThreadedProducer` and `ThreadedConsumer`.  The purpose of background producer and consumer is to better test the failure case to avoid serialization (produce -> broker restart -> consumer) which can not reveal the insight that consumers consume the same records more than once when broker failure happens.
   
        (e) `MirrorConnectorsIntegrationSSLTest`: This is a **new** test and it should be pretty lean and neat.
   
   In addition, the **new** base test class `MirrorConnectorsIntegrationBaseTest` follows the existing practice of K-stream integration test structure https://github.com/apache/kafka/blob/trunk/streams/src/test/java/org/apache/kafka/streams/integration/ResetIntegrationWithSslTest.java#L37 so I assume it is likely the right way tp structure tests with more than one scenarios (SSL v.s. non-SSL)
   
   If we have to control the complexity, I would prefer to drop `testWithBrokerRestart` and keep `MirrorConnectorsIntegrationSSLTest`, as it makes sense to run simple validation in SSL setup.
   
   Thanks and would like to hear more thoughts and feedback. 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org