cmccabe commented on pull request #11417: URL: https://github.com/apache/kafka/pull/11417#issuecomment-952366646
> The patch seems ok. I confess it's not obvious to me what this approach buys us over the use of ClusterTestExtensions. Seems like another way we could do this is to let QuorumTestHarness make use of ClusterTestExtensions instead of duplicating all the logic to build the cluster. That would still let us make minor modifications to the extensions in order to enable it. As it is, we're left with two separate approaches for covering kraft and zk which seems less than ideal. It might be fine, but it would be nice to understand what was the gap with ClusterTestExtensions and whether we should fix it. I did consider reusing one of the other test frameworks, but there just isn't a lot to be gained because of the radically different approach that existing tests take. The code for creating the `BrokerServer` / `ControllerServer` is actually a VERY tiny part of this PR... the majority of the PR is integrating with stuff like the `@BeforeEach` functions in each test, handling the way the control flow bounces around the inheritance hierarchy, etc. The goal here is to be able to convert existing tests without rewriting them. I have actually done this for about a dozen tests, which I haven't posted here since this PR is big enough. I think this is the only approach that will get us over the finish line for kraft. The effort to move to a saner test structure, not based on N levels of inheritance (which partially duplicate each others' functionality), is a noble goal, but I think it has to be a separate goal if we want to succeed. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
