KAFKA-15853: Move and rewrite CoreUtils to Java in server-common #21289
Conversation
github-actions
bot
added
triage
| // see server-common.server.util | ||
| // see LockUtils#inLock(Lock lock, ThrowingSupplier<T, E> supplier) | ||
| // see LockUtils#inLock(Lock lock, ThrowingRunnable<E> runnable) | ||
| def inLock[T](lock: Lock)(fun: => T): T = { |
There was a problem hiding this comment.
This method was already moved to LockUtils
- see LockUtils#inLock(Lock lock, ThrowingSupplier<T, E> supplier)
- see LockUtils#inLock(Lock lock, ThrowingRunnable runnable)
UladzislauBlok
changed the title
mimaison
left a comment
mimaison
left a comment
There was a problem hiding this comment.
Thanks for the PR. I think we may be able to get rid of CoreUtils altogether.
| * 2. It is the most general possible utility, not just the thing you needed in one particular place | ||
| * 3. You have tests for it if it is nontrivial in any way | ||
| */ | ||
| public class CoreUtils { |
There was a problem hiding this comment.
The class name does not make sense anymore, as it's not in the core module anymore.
| * @param logging The logging instance to use for logging the thrown exception. | ||
| * @param logLevel The log level to use for logging. | ||
| */ | ||
| public static void swallow(Runnable action, Logger logging, Level logLevel) { |
There was a problem hiding this comment.
Do we really need this method? Could we use Utils.closeQuietly() instead?
There was a problem hiding this comment.
I didn't know about Utils.closeQuietly(). I can see that sometimes swallow is used with ERROR level, but we should rather update Utils.closeQuietly() to support other levels instead of add duplicated functionality here.
| * | ||
| * @param files list of files to be deleted | ||
| */ | ||
| public static void delete(List<String> files) throws IOException { |
There was a problem hiding this comment.
This is only used by tests so we shouldn't have this in src/main. We could move it to TestUtils or even inline it as it's really short, it could even be a 1 liner if using the functional syntax forEach().
| } | ||
| } | ||
|
|
||
| public static List<Endpoint> listenerListToEndPoints(List<String> listeners, Map<ListenerName, SecurityProtocol> securityProtocolMap) { |
There was a problem hiding this comment.
All the methods below are only used by KafkaConfig so they should probably be moved there.
| * @param name The name to register this mbean with | ||
| * @return true if the registration succeeded | ||
| */ | ||
| public static boolean registerMBean(Object mbean, String name) { |
There was a problem hiding this comment.
See my other comments, but it seems all other methods could be deleted or moved elsewhere. That leaves us with only this method. It does not make sense to have a utility class for a single method.
There was a problem hiding this comment.
I think we can move it to Utils
|
@mimaison Hey. Thanks for the comments. I agree with you on the point that we can refactor method to use it inline (like delete) or move it to other classes like |
2 participants
CoreUtils.scalais currently a blocker for migratingKafkaConfig.scalato the server-common module. This PR introduces a Java implementation ofCoreUtilswithin server-common to facilitate the ongoing effort of decoupling the server modules.Key Changes:
CoreUtils.scalainto a newCoreUtils.javaclass, located in theorg.apache.kafka.server.utilpackage insideserver-commonmodule.SocketServerConfigsto the server-common module, as it is a required dependency for the migrated utility methods.LockUtils. see PR #19961. With the adjustments for read/write locks.Architecture Note: I have moved
SocketServerConfigsto server-common to resolve dependency issues. Currently, configuration classes are split between server and server-common. I'd like to clarify what is the strategic place to keep config related classes?Migration Strategy: To keep the scope of this PR manageable and ensure a smoother review process,
CoreUtils.scalahas not been removed. This PR focuses on providing the new Java-based infrastructure. A subsequent PR will be submitted to replace existing call sites and perform the final cleanup of the Scala implementation.