spring-cloud / spring-cloud-stream

Framework for building Event-Driven Microservices
http://cloud.spring.io/spring-cloud-stream
Apache License 2.0
1.01k stars 614 forks source link

Pulsar binder integ test failure on JDK 21 #2915

Closed sobychacko closed 8 months ago

sobychacko commented 8 months ago

PulsarBinderIntegrationTests#binderAndBindingPropsAreAppliedAndRespected is failing on JDK 21 because of some issues with the property max-pending-messages. Please investigate, why this property has issues on JDK 21, but not on JDK 17.

onobc commented 8 months ago

TL;DR

The error has been there from inception and is only noticed because the ProducerConfigurationData.setMaxPendingMessages and ProducerConfigurationData.setMaxPendingMessagesAcrossPartitions are called in different order in Java17 and 21.

If the failing test would have instead set only the maxPendingMessagesAcrossPartitions we would have seen the same issue in Java17.

Details

The binder allows users to configure the underlying producer used by the binder.

The config props are layered from base -> binder -> binding and we only include those properties that have changed from the defaults.

When hydrating the ProducerConfigurationData with the modified props, the setMaxPendingMessagesAcrossPartitions validates that it is >= the maxPendingMessages.

In the case where only the maxPendingMessages is set, the maxPendingMessagesAcrossPartitions has a default of 0. If the setMaxPendingMessagesAcrossPartitions is called after setMaxPendingMessages then the validation will fail.

In Java17, the serialized JSON and merged set of props were put into a HashMap which just so happened to result in maxPendingMessagesAcrossPartitions being called before maxPendingMessages and we did not notice the issue in the test.

In Java21, the resulting props in the HashMap just so happen to result in maxPendingMessages being called before maxPendingMessagesAcrossPartitions and we see the validation error.

Solution

We are limiting the props we customize to only those set by a user. The main reason for this was due to complication when using multiple layers.

layer1.max: 5 (default)
layer2.max: 7
layer2.max: 5 (default)

If we do layer1.put(layer2).put(layer3) we end up w/ "max = 5" which is not what we want.

A better approach is to do 2 passes w/ the 1st pass only setting those that are overridden and the 2nd pass going back and if there is a default property that is not present then put the default value.

This solves this current case and any future ones where our defaults in ProducerConfigProperties are not the same as the ones in ProducerConfigurationData.