[
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)
