GH-2870: EmbeddedKafka: register BeanDefinition

[artembilan]

Fixes #2870

The EmbeddedKafkaContextCustomizer uses beanFactory.initializeBean() which is too early according to the ApplicationContext lifecycle since it is not refreshed yet for ContextCustomizer

• Rework the logic in the EmbeddedKafkaContextCustomizer to register a BeanDefinition for an EmbeddedKafkaBroker to include it into standard ApplicationContext lifecycle
• Also mark @EmbeddedKafka with a @DisabledInAotMode to disallow this kind of tests in native images

[artembilan]

OK. So, there is a problem with this fix.
After starting that embedded broker we expose several system props:

System.setProperty(SPRING_EMBEDDED_KAFKA_BROKERS, getBrokersAsString());
System.setProperty(this.brokerListProperty, getBrokersAsString());

The in test configuration this cannot be resolved:

		@Value("${" + EmbeddedKafkaBroker.SPRING_EMBEDDED_KAFKA_BROKERS + "}")
		private String brokerAddresses;

Just because we have not initialized environment yet.
Therefore trying to postpone an embedded broker initialization from that customizer leads to many other problems, where the mentioned afterPropertiesSet() is not a problem at all for the rest of application context used just in a test scope. 🤷

[snicoll]

IMO, there is no rush to fix this, certainly post RC as I expect other lifecycle issues. The code that we have now is making sure that the broker starts (very) early and, from that perspective, it is a good thing.

where the mentioned afterPropertiesSet() is not a problem at all for the rest of application context used just in a test scope.

Having it working doesn't necessarily means it's right. To make it right, the bits that mutate the environment should be separated from the bits that actually start the container. Then, to make sure things are initialized in the right order, a dependency should probably be defined between the embedded broker and whatever is using it first. At the moment, the broker became a bean like any other bean and the container is free to start it whenever it feels like.

I can understand why it's frustrating to have to spend more time to make it work again with the proper semantics but having it done initially wouldn't have lead to this issue at all.

[garyrussell]

I would rather change the tests to get the broker address from the broker bean itself rather than using that @Value injection.

We should not be calling afterPropetiesSet() in the customizer - that's what initializeBean() did before.

[artembilan]

There is more into that, Gary.
Our System.setProperty(this.brokerListProperty, getBrokersAsString()); is based on the spring.kafka.bootstrap-servers and it contributes to Spring Boot auto-configuration.
Therefore, as Stephane said, the environment must be mutated before the application context is refreshed.
From here we cannot defer that afterPropetiesSet() to the application context lifecycle.

I guess if we would not expose this EmbeddedKafkaBroker as a bean, but just started it from the customizer as it is done with many other embedded services, then we would not have this discussion at all.
Well, in fact, when there is no Spring context around the test, we use an EmbeddedKafkaCondition logic based on JUnit lifecycle.

In the end this customizer is there really to expose a bean for target Spring configuration convenience in the test, but the Kafka server must be started and its options exposed to the environment before other beans got into the action.
The mix of concerns in the EmbeddedKafkaBroker implementation is really one-stop-shop for end-user convenience.
You may even not use any annotations, but just declare that EmbeddedKafkaBroker as a bean in your test configuration.
Or just plain:

	@BeforeAll
	static void setup() {
		embeddedKafka = new EmbeddedKafkaKraftBroker(1, 2, topic1, topic2, topic3, topic4, topic5, topic6);
		embeddedKafka.afterPropertiesSet();

So, why is that so bad to have a single convenient component which can be reused in different places? The end result is very beneficial for users of our framework.
Finally, we just talk about the test environment and it has no any effect on production code 🤷