[ https://issues.apache.org/jira/browse/KAFKA-17174?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17868460#comment-17868460 ]
Chris Egerton commented on KAFKA-17174: --------------------------------------- {quote}{color:#172b4d}Unfortunately, fragment must happen since connect is a huge module. It is hard to address migration in few PRs. We have similar experience in migrating mock tool and updating junit, right{color} {quote} I completely agree that some fragmentation will be unavoidable; this is why I included the second part of that quote: "unless we're in the midst of a short migration period where the work is broken up into several smaller PRs, some of which have been merged and some of which have not". The difference between this and Mockito/JUnit is that the requirements for migrating testing frameworks are fairly straightforward and known in advance. I'm just saying that there are a few unknowns with the migration to a new embedded cluster API that I think we should identify and settle on before we make any changes in that direction. I'm also a little insistent on this right now because I'm actively writing, reviewing, and maintaining the Connect and MM2 integration tests at the moment and I'll be one of the people absorbing the brunt of the additional workload incurred by the migration if it drags on and we have an extended period of fragmentation. Maybe we can settle on a tentative goal of at most two months for the migration period, with no work starting before a high-level plan has been created that covers all of the anticipated changes we'll need to implement for the {{ClusterInstance}} and {{EmbeddedConnect}} APIs to cover currently-unbridged gaps? RE the in-/out-of-band cluster starting API, my point was that I don't want to change from something like this: {code:java} new EmbeddedConnectCluster.Builder() .build(); {code} to this: {code:java} new EmbeddedConnectCluster.Builder() .kafkaCluster(someClusterInstanceStartedByClusterExtensionsApi) .build();{code} (This is what I believe you were describing with "3. `EmbeddedConnect` will have another constructor which can take `ClusterInstance` as argument"). This would be a breaking change for a lot of external users, and while our testing API isn't public, I don't think there's a good enough reason to incur this change, especially since it'd be less convenient even within the scope of the Kafka project itself. If possible, I'd like to keep the internal API where starting/stopping a Connect cluster automatically starts/stops the underlying Kafka cluster. I think this would require changes to the {{ClusterInstance}} API to allow programmatic construction of a {{{}ClusterInstance{}}}, so that in our embedded Connect cluster API we programmatically bring up a {{{}ClusterInstance{}}}, instead of using the {{ClusterTestExtensions}} API to automatically bring it up at the beginning of each test. If a primary goal for switching over to the {{ClusterTestExtensions}} API is to get other benefits like automatic detection of thread leaks, then a suitable compromise could be to extract the logic for these auxiliary benefits into a separate extension that can be leveraged in Connect tests on its own. BTW, I think a large pain point here is that it's difficult to compose JUnit extensions that automatically inject parameters. AFAICT there's no way for, e.g., a {{ConnectClusterTestExtensions}} class to be aware of any {{ClusterInstance}} object that will be automatically injected into a test class/method, and then leverage that object to bring up its own embedded Connect cluster. Maybe we could do something to make the {{ClusterTestExtensions}} class inheritance-friendly, so that users could extend that class instead of annotating their test class with both {{@ExtendWith(ClusterTestExtensions.class)}} and {{{}@ExtendWith(ConnectClusterTestExtensions.class){}}}? > Migrate OffsetsApiIntegrationTest to use ClusterTestExtensions > -------------------------------------------------------------- > > Key: KAFKA-17174 > URL: https://issues.apache.org/jira/browse/KAFKA-17174 > Project: Kafka > Issue Type: Improvement > Reporter: Chia-Ping Tsai > Assignee: Chia-Ping Tsai > Priority: Minor > > Connect module has its `EmbeddedKafkaCluster`, so another alternative is to > re-implement it by `ClusterInstance`. However, both of them are a kind of > "embedded service" interface. Personally, I prefer to unify the interface for > all modules in testing. We can move any useful helpers from > `EmbeddedKafkaCluster` to `ClusterInstance` in order to make other modules > get benefits too. > `OffsetsApiIntegrationTest` is a lightweight user of `EmbeddedKafkaCluster`, > so it can be a good entrypoint to introduce `ClusterInstance` to connect > module -- This message was sent by Atlassian Jira (v8.20.10#820010)