GH-2359: Initial AOT Support for Native #2364

garyrussell · 2022-07-21T16:46:11Z

Resolves #2359

This is sufficient for the basic Kafka smoke test.

I will issue another PR if changes are needed for the Streams and Avro versions.
I have a KafkaAvroBeanRegistrationAotProcessor but I have omitted it from
this commit because it hasn't been tested.

Resolves spring-projects#2359 This is sufficient for the basic Kafka smoke test. I will issue another PR if changes are needed for the Streams and Avro versions. I have a `KafkaAvroBeanRegistrationAotProcessor` but I have omitted it from this commit because it hasn't been tested.

artembilan

I wonder if Avro stuff has to be somewhere else: doesn't look like it is just Kafka-specific...

artembilan · 2022-07-21T16:55:06Z

spring-kafka/src/main/java/org/springframework/kafka/aot/KafkaRuntimeHintsRegistrar.java

+		hints.proxies().registerJdkProxy(Consumer.class, SpringProxy.class, Advised.class, DecoratingProxy.class);
+		hints.proxies().registerJdkProxy(Producer.class, SpringProxy.class, Advised.class, DecoratingProxy.class);
+
+		if (ClassUtils.isPresent("org.apache.kafka.streams.StreamsBuilder", null)) {


I guess it is preferable to use a dedicated for this purposes API on the hints: registerTypeIfPresent()

The problem with that is I have to use FQ class names instead of the classes themselves.

That is correct: there is a variant for fqcn and classLoader

Are you going to fix this via onReachableType(TypeReference.of("org.apache.kafka.streams.StreamsBuilder") ?

That is correct: there is a variant for fqcn and classLoader

Right converting all those classes to FQCN is a pain.

Where is onReachableType?

See how I used it in my SI PR: https://github.com/spring-projects/spring-integration/pull/3832/files#diff-b471878878aa8838786d4a93b19f8014d497db6ace582572f834da814720e01bR97

artembilan · 2022-07-21T16:56:10Z

spring-kafka/src/main/java/org/springframework/kafka/aot/KafkaRuntimeHintsRegistrar.java

+					ConsumerProperties.class,
+					ContainerProperties.class,
+					ProducerListener.class,
+					KafkaListener.class,


I believe annotations must be registers with their special API like you did in Spring AMQP.

artembilan · 2022-07-21T16:56:33Z

spring-kafka/src/main/java/org/springframework/kafka/aot/KafkaRuntimeHintsRegistrar.java

+					ContainerProperties.class,
+					ProducerListener.class,
+					KafkaListener.class,
+					EnableKafka.class)


I don't think we need this annotation to expose...

artembilan · 2022-07-21T16:58:11Z

spring-kafka/src/main/java/org/springframework/kafka/aot/KafkaRuntimeHintsRegistrar.java

+								MemberCategory.INTROSPECT_PUBLIC_METHODS)));
+
+		Stream.of(
+					Message.class,


Looks like you have duplicated this type.
I also wonder if we really use it via reflection at all...
In SI I registered it only for serialization.

Again, I just copied what was in spring-native, for now.

spring-kafka/src/main/java/org/springframework/kafka/aot/KafkaRuntimeHintsRegistrar.java

garyrussell · 2022-07-21T17:07:07Z

The avro processor is specific to spring-kafka because it discovers listeners that use Avro types and registers them for reflection.

I left it out of this PR to avoid any confusion (and because I haven't tested it yet).

…ctories.

artembilan · 2022-07-21T17:36:18Z

spring-kafka/src/main/java/org/springframework/kafka/aot/KafkaRuntimeHintsRegistrar.java

+		RuntimeHintsUtils.registerAnnotation(hints, EnableKafka.class);
+		RuntimeHintsUtils.registerAnnotation(hints, PartitionOffset.class);
+		RuntimeHintsUtils.registerAnnotation(hints, TopicPartition.class);
+		RuntimeHintsUtils.registerAnnotation(hints, MessageMapping.class);


This annotation is registered by Spring Framework by itself. See MessagingAnnotationsRuntimeHints

artembilan · 2022-07-21T17:38:24Z

spring-kafka/src/main/java/org/springframework/kafka/aot/KafkaRuntimeHintsRegistrar.java

@@ -115,24 +116,23 @@ public class KafkaRuntimeHintsRegistrar implements RuntimeHintsRegistrar {

 	@Override
 	public void registerHints(RuntimeHints hints, @Nullable ClassLoader classLoader) {
+		RuntimeHintsUtils.registerAnnotation(hints, KafkaListener.class);
+		RuntimeHintsUtils.registerAnnotation(hints, KafkaListeners.class);
+		RuntimeHintsUtils.registerAnnotation(hints, EnableKafka.class);


I still don't think we need this. The @Import processing is initiated by AOT. So, that our KafkaBootstrapConfiguration is already registered at runtime.
Therefore I don't see a reason in this registration.

artembilan · 2022-07-21T17:50:56Z

spring-kafka/src/main/java/org/springframework/kafka/aot/KafkaRuntimeHintsRegistrar.java

+				.forEach(type -> reflectionHints.registerType(type, builder ->
+						builder.withMembers(MemberCategory.INVOKE_PUBLIC_CONSTRUCTORS)));
+
+		reflectionHints.registerType(TypeReference.of("java.util.zip.CRC32C"), builder ->


Why FQCN? Look like this class is a part of Java by itself

Again; copied from spring-native.

artembilan · 2022-07-21T17:52:23Z

spring-kafka/src/main/java/org/springframework/kafka/aot/KafkaRuntimeHintsRegistrar.java

+		hints.proxies().registerJdkProxy(Consumer.class, SpringProxy.class, Advised.class, DecoratingProxy.class);
+		hints.proxies().registerJdkProxy(Producer.class, SpringProxy.class, Advised.class, DecoratingProxy.class);
+
+		if (ClassUtils.isPresent("org.apache.kafka.streams.StreamsBuilder", null)) {


Are you going to fix this via onReachableType(TypeReference.of("org.apache.kafka.streams.StreamsBuilder") ?

artembilan · 2022-07-21T17:53:02Z

spring-kafka/src/main/java/org/springframework/kafka/aot/KafkaRuntimeHintsRegistrar.java

+				builder.withMembers(MemberCategory.INVOKE_PUBLIC_CONSTRUCTORS));
+
+		hints.proxies().registerJdkProxy(Consumer.class, SpringProxy.class, Advised.class, DecoratingProxy.class);
+		hints.proxies().registerJdkProxy(Producer.class, SpringProxy.class, Advised.class, DecoratingProxy.class);


AopProxyUtils.completeJdkProxyInterfaces() ?

This reverts commit 2e7a084.

artembilan · 2022-07-25T20:53:55Z

...kafka/src/main/java/org/springframework/kafka/aot/KafkaAvroBeanRegistrationAotProcessor.java

+
+	private static boolean isListener(Class<?> beanType) {
+		return GenericMessageListener.class.isAssignableFrom(beanType)
+				|| MergedAnnotations.from(beanType).get(KAFKA_LISTENER_CLASS_NAME).isPresent();


Maybe just MergedAnnotations.isPresent(String annotationType) ?

Actually, it turns out we don't need to process the annotations, only GenericMessageListeners that use Avro types - the ones that are method parameters are already discovered.

…stener` only `GenericMessageListener`.

artembilan

LGTM

Let me know if anything else you'd like to add to this PR or it is good to go for merging!

garyrussell · 2022-07-27T15:15:23Z

Thanks; all smoke tests are green, but I will add a couple more before merging this.

garyrussell · 2022-07-27T16:43:50Z

Ok to merge after this final commit; thanks!

artembilan requested changes Jul 21, 2022

View reviewed changes

Remove duplicates; register annotations properly; remove avro from fa…

1271ac3

…ctories.

artembilan requested changes Jul 21, 2022

View reviewed changes

Remove @MessageMapping, @EnableKafka.

d28767a

artembilan requested changes Jul 21, 2022

View reviewed changes

garyrussell added 6 commits July 22, 2022 14:48

Fix Spring proxies.

7827dc5

Add KafkaAvroBeanRegistrationAotProcessor to detect Avro types.

62e5ee1

Use registerTypeIfPresent() for Kafka Streams.

7aa5a5b

Diags.

2e7a084

Polish Avro detection.

2ccc7d5

Revert "Diags."

0933e04

This reverts commit 2e7a084.

artembilan requested changes Jul 25, 2022

View reviewed changes

garyrussell added 2 commits July 25, 2022 17:16

Polish use of MergedAnnotations.

c405f92

Kafka Avro AOT Processor polishing; no need to register for `@KafkaLi…

9ff9b01

…stener` only `GenericMessageListener`.

artembilan approved these changes Jul 25, 2022

View reviewed changes

Fix BatchMessageListener (List<ConsumerRecord<Foo, Bar>>).

cefe09c

artembilan merged commit 3d8dcce into spring-projects:main Jul 27, 2022

GH-2359: Initial AOT Support for Native #2364

GH-2359: Initial AOT Support for Native #2364

Uh oh!

Conversation

garyrussell commented Jul 21, 2022

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

garyrussell commented Jul 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

garyrussell commented Jul 27, 2022

Uh oh!

garyrussell commented Jul 27, 2022

Uh oh!

Uh oh!