-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-3662: Support Kafka 3.9.0+ in EmbeddedKafkaKraftBroker #3665
Conversation
Fixes: spring-projects#3662 Issue: spring-projects#3662 Add compatibility for both Kafka 3.8.0 and 3.9.0+ by handling different method signatures for setConfigProp: - 3.9.0+: setConfigProp(String, Object) - 3.8.0: setConfigProp(String, String) The change uses reflection to detect Kafka version and call appropriate method.
} | ||
else { | ||
// For Kafka 3.8.0: setConfigProp(String, String) | ||
Method setConfigMethod = builderClass.getMethod("setConfigProp", String.class, String.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need a reflection for this method?
Please, consider to find method via reflection only one in static
block of the class.
|
||
private static boolean isClassicConsumerPresent() { | ||
try { | ||
Class.forName(CLASS_EXISTS_ONLY_IN_390); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See ClassUtils.isPresent()
.
if (isKafka39OrLater) { | ||
// For Kafka 3.9.0+: setConfigProp(String, Object) | ||
Method setConfigMethod = builderClass.getMethod("setConfigProp", String.class, Object.class); | ||
setConfigMethod.invoke(clusterBuilder, (String) key, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also see ReflectionUtils.findMethod()
and ReflectionUtils.invokeMethod()
.
You won't need to deal with any exceptions then!
private static final Method SET_CONFIG_METHOD; | ||
|
||
static { | ||
IS_KAFKA_39_OR_LATER = ClassUtils.isPresent(CLASS_EXISTS_ONLY_IN_390, EmbeddedKafkaKraftBroker.class.getClassLoader()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isPresent()
doesn't throw an exception: can be used directly in the property declaration line.
Also: I don't see a reason in the CLASS_EXISTS_ONLY_IN_390
constant. You simply can use that class name in this method call.
@@ -243,6 +266,17 @@ private void start() { | |||
System.setProperty(SPRING_EMBEDDED_KAFKA_BROKERS, getBrokersAsString()); | |||
} | |||
|
|||
private static void setConfigProperty(KafkaClusterTestKit.Builder clusterBuilder, Object key, Object value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit-pick: can we cast key
into a String
where we call this method?
That way that cast operation would happen only in one place.
Fixes: #3662
Issue: #3662
Add compatibility for both Kafka 3.8.0 and 3.9.0+ by handling different method signatures for setConfigProp:
The change uses reflection to detect Kafka version and call appropriate method.