ning2008wisc commented 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 the topic config
validation is trivial, I am 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) by decoupling the producer thread, embedded kafka and consumer
thread.
(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 structure to extend from a base class.
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:
us...@infra.apache.org
