Skip to content
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

Quarkus Redis client timeout shorter than a second #15194

Closed
abutic opened this issue Feb 19, 2021 · 5 comments · Fixed by #15211
Closed

Quarkus Redis client timeout shorter than a second #15194

abutic opened this issue Feb 19, 2021 · 5 comments · Fixed by #15211
Labels
area/redis good first issue Good for newcomers kind/bug Something isn't working
Milestone

Comments

@abutic
Copy link

abutic commented Feb 19, 2021

Describe the bug
The docs says Redis client timeout is defined as a Duration:
https://quarkus.io/guides/redis#quarkus-redis-client_quarkus.redis.timeout

A Duration object is later passed to an underlying implementation, as can be seen here:

io.vertx.mutiny.redis.client.Response response = mutinyResponse.await().atMost(Duration.ofSeconds(timeout));

but, internally, only seconds precision is used:

, with the timeout parameter representing the seconds.

So, without fractional seconds precision, we lost the ability to have timeouts shorter than a second.

Expected behavior
Instead of silently discarding fractional seconds part, which later leads to a misleading error when client is used:

java.lang.IllegalArgumentException: `duration` must be greater than zero

, it would be nice to allow timeouts shorter than a second, by storing the Duration internally and later passing it as is to Mutiny Redis Client.

The alternative is to throw a more precise exception, saying for instance that timeouts shorter than a second, even though they're allowed by the Duration, are not allowed.

To Reproduce
Define a timeout shorter than a second and try using a RedisClient. E.g.:

quarkus.redis.timeout=PT0.1S
@abutic abutic added the kind/bug Something isn't working label Feb 19, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Feb 19, 2021

/cc @cescoffier, @gsmet, @machi1990

@gsmet
Copy link
Member

gsmet commented Feb 19, 2021

@abutic would you be interested in providing a pull request? Looks like you did most of the work already :).

@gsmet gsmet added the good first issue Good for newcomers label Feb 19, 2021
@abutic
Copy link
Author

abutic commented Feb 20, 2021

@abutic would you be interested in providing a pull request? Looks like you did most of the work already :).

I was "afraid" you'd ask... :)

PR: #15211

@abutic
Copy link
Author

abutic commented Feb 22, 2021

@gsmet and @machi1990, should I close this issue, now that it's fixed by #15211? Also, should the PR be linked to this issue (I don't seem to have the permission to link them)?

@geoand
Copy link
Contributor

geoand commented Feb 22, 2021

@abutic I'll close it.

Just as an FYI for next time, if you add Fixes: #NNNNN like I have done in the description of your PR, GH will automatically link the PR to the issue :)

@geoand geoand closed this as completed Feb 22, 2021
@geoand geoand added this to the 1.13 - master milestone Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/redis good first issue Good for newcomers kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants