quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.64k stars 2.64k forks source link

Quarkus Redis extension does not allow empty redis.hosts property #43606

Open The-Funk opened 2 days ago

The-Funk commented 2 days ago

Describe the bug

Redis DevServices have been fixed to start if the redis.hosts property is null OR empty. However in the case of empty, even though the dev services have started, the Redis extension throws an exception because the hosts value is empty.

This is needed for people like me who have ephemeral GitHub Actions runners for their CI that live inside a K8s cluster and cannot run docker themselves. These runners must be given an actual host value when running tests, and so the CI environment has been given real redis instances. However when testing locally on a laptop, or on a VM, docker is available, and as such there's no need to give redis.hosts a value or point to a remote instance of redis. This is useful because of corporate firewalling which makes accessing a remote service rather painful.

All of this leads to an interesting conundrum. What if you need a test container for local dev and a physical instance for remote CI, and both tests should run under the same profile for guaranteed consistency?

Expected behavior

If Redis hosts property is null OR empty, dev services should start If redis hosts present OR dev services started, Quarkus Redis extension should not throw.

Actual behavior

Redis DevServices have been fixed to start if the redis.hosts property is null OR empty. However in the case of empty, even though the dev services have started, the Redis extension throws an exception because the hosts value is empty. This causes the Quarkus application to fail to start.

How to Reproduce?

No response

Output of uname -a or ver

Windows 11

Output of java -version

21

Quarkus version or git rev

2.45.1

Build tool (ie. output of mvnw --version or gradlew --version)

3.9.8

Additional information

No response

quarkus-bot[bot] commented 2 days ago

/cc @Ladicek (redis), @cescoffier (redis), @machi1990 (redis)

cescoffier commented 2 days ago

Fancy a PR?

The-Funk commented 2 days ago

@cescoffier will give it the old college try!

The-Funk commented 1 day ago

@cescoffier This might be a little outside my pay range but wanted to get your thoughts.

https://github.com/quarkusio/quarkus/blob/7aa6f66917bf781116bbf5d96690482acc6eefa9/extensions/redis-client/runtime/src/main/java/io/quarkus/redis/runtime/client/VertxRedisClientFactory.java#L68

I found the specific area in code where this happens. All of this is in the quarkus-redis-client extension, under runtime/client.

In the file linked above, if redis hosts or redis.hosts-provider-name is configured, even if it is empty, this factory is invoked in order to create the client(s). You could have one client or many named clients or annotated clients.

In the RedisClientProcessor, lines 138 to 158 gather those client names and config details to be sent to the factory for creation, followed by line 161 which then initializes the client.

My thought is, if there's an easy way to determine if a Redis client bean exists already with a given name, signifying that the bean was already created by dev services, we would simply ignore calling initialize on line 161 for those names, this includes the default client too.

We could make this change even more intentional if this information could be directly queried from dev services. That would probably be best because then we could check if dev services is enabled prior to filtering out names of clients that already exist, reducing the risk of a regression where a bean might exist temporarily in the container but is then removed for some reason, and thus...failure.

Is there an easy way in the RedisClientProcessor class that we could query dev services to determine if it has initialized any named beans/default beans for a class or is the only solution for that a direct call to the ArC?