MINOR: Migrate CoordinatorLoaderImplTest from Scala to Java #20089
Conversation
github-actions
bot
added
triage
| @@ -1660,9 +1660,11 @@ project(':coordinator-common') { | |||
| implementation libs.slf4jApi | |||
| implementation libs.metrics | |||
| implementation libs.hdrHistogram | |||
| implementation libs.scalaLibrary | |||
There was a problem hiding this comment.
I don't think we should add scalaLibrary to the :coordinator-common module.
There was a problem hiding this comment.
Thank you for the feedback.
| <allow pkg="org.apache.kafka.server.util" /> | ||
| <allow pkg="org.apache.kafka.storage.internals.log" /> | ||
| <allow pkg="org.apache.kafka.test" /> | ||
| <allow pkg="org.apache.kafka.timeline" /> | ||
| <allow pkg="org.HdrHistogram" /> | ||
| <allow pkg="scala" /> |
There was a problem hiding this comment.
Thank you for the feedback. I’m keeping coordinator-common as a pure Java module, but since the tests depend on the Scala-based ReplicaManager, we still need scala, like OptionConverters in test scope.
There was a problem hiding this comment.
Would you prefer that we wait until ReplicaManager is refactored into Java before removing all Scala dependencies from this module, or would it make more sense to move these tests elsewhere, or wrap the Scala APIs in Java-friendly facade?
There was a problem hiding this comment.
Yes, ReplicaManager is needed here so we need to wait @chia7712 feedback.
| */ | ||
| public class CoordinatorLoaderImpl<T> implements CoordinatorLoader<T> { | ||
|
|
||
| private static final Logger LOG = LoggerFactory.getLogger(CoordinatorLoader.class); |
There was a problem hiding this comment.
Should the logger class be CoordinatorLoaderImpl?
There was a problem hiding this comment.
Thank you for the feedback.
| <!-- no one depends on the server --> | ||
| <disallow pkg="kafka" /> |
There was a problem hiding this comment.
Should we do this change?
From my understanding, this prevents developer to depend on core module again in migrating process.
Summary of Changes
CoordinatorLoaderImplandCoordinatorLoaderImplTestin Java, replacing their original Scala implementations.ReplicaManagerand replaced it with functional interfaces forpartitionLogSupplierandpartitionLogEndOffsetSupplier