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:
[email protected]
