Skip to content

Conversation

@kdubb
Copy link
Contributor

@kdubb kdubb commented Jul 25, 2023

This enables configuration of the new maxLifetime value provided by Vert.x 4.4.2 that ensures connections are recycled after a maximum duration.

Because maxLifetime could conceivable be set beyond the maximum value that milliseconds can store in an int (24.8 days) I created a UnitisedTime class. The UnitisedTime class has a unitised method that converts a Duration into the smallest possible TimeUnit that can be stored in an int, starting with milliseconds.

For consistency, I updated the idleTimeout to use the same UnitisedTime.unitised utitlity for conversion.

This was seemingly missed in PR #31873 and is required for use with the Vault Credentials Provider. To this end, I added a blurb about "Time Limited Credentials" to the Credentials Provider docs.

@kdubb
Copy link
Contributor Author

kdubb commented Jul 25, 2023

I can't find where tests are for things like idle-timeout are located. I'm assuming that there should be a place to test the UnitisedTest.unitise method and that configuration properties like idle-timeout and max-lifetime applied to the pool implementations correctly.

@github-actions
Copy link

github-actions bot commented Jul 25, 2023

🙈 The PR is closed and the preview is expired.

@quarkus-bot

This comment has been minimized.

@tsegismont
Copy link
Contributor

Because maxLifetime could conceivable be set beyond the maximum value that milliseconds can store in an int (24.8 days) I created a UnitisedTime class. The UnitisedTime class has a unitised method that converts a Duration into the smallest possible TimeUnit that can be stored in an int, starting with milliseconds.

This new class is not necessary. In Quarkus, all config properties defined as Duration can take a string value following the standard format: https://docs.oracle.com/javase/8/docs/api/java/time/Duration.html#parse-java.lang.CharSequence-

Copy link
Contributor

@tsegismont tsegismont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @kdubb

See my other comment about unitised time


=== RabbitMQ

When using the `smallrye-reactive-messaging-rabbitmq` extension there is no configuration needed. The
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, what's different with the RabbitMQ extension? If the CredentialsProvider has the "expires-at property, couldn't we use the value to configure the pool correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RabbitMQ connections supports this automatically, it has built support for dynamic credentials.

Vert.x connection pooling does not support this. I made the PR to add max-lifetime to the Vert.x pool and in its current state I believe it would be hard to support this type of feature. Currently the SqlConnectionPool doesn't allow configuration of per-connection properties. If we gain the ability to set properties (specifically something like max lifetime) per connection this could be implemented

@kdubb
Copy link
Contributor Author

kdubb commented Jul 25, 2023

This new class is not necessary. In Quarkus, all config properties defined as Duration can take a string value following the standard format.

@tsegismont I am using that feature, and it is currently used for idle-timeout, but the Vert.x pool takes an int and a TimeUnit for time values; not a duration. Some method of translating between them is necessary.

Previously idle-timeout was converting the duration to long milliseconds and using Math.toIntExact to convert the milliseconds to an int; which throws an ArtithmeticException on overflow. This meant that the max duration it could accept was ~24.8 days, if larger the app would fail with the arithmetic exception.

The unitised utility picks the smallest non-overflowing TimeUnit and uses that. Using the utility allows the configuration duration essentially be unbounded (which is good for max-lifetime). Also, it removes what might be a very "odd" exception on boot if an overflowing time was configured.

@quarkus-bot

This comment has been minimized.

gsmet
gsmet previously requested changes Jul 31, 2023
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need a squash (thinking of the last commit).

Also, I would like to wait for the big Vert.x/datasource/config PR to go in before we rebase and merge this one as this one will conflict with it.

@kdubb
Copy link
Contributor Author

kdubb commented Jul 31, 2023

@gsmet Which PR is that?

@kdubb kdubb force-pushed the feature/reactive-sql-max-lifetime branch from fdbb81b to be79d01 Compare July 31, 2023 19:00
@kdubb
Copy link
Contributor Author

kdubb commented Jul 31, 2023

Squashed & rabased. Left the separate doc update as a separate commit.

@quarkus-bot

This comment has been minimized.

@gsmet
Copy link
Member

gsmet commented Aug 1, 2023

@kdubb that would be this small one: #33228

I'll rebase yours once it is in.

@tsegismont
Copy link
Contributor

@tsegismont I am using that feature, and it is currently used for idle-timeout, but the Vert.x pool takes an int and a TimeUnit for time values; not a duration. Some method of translating between them is necessary.

Ok, thanks for the details.

Copy link
Contributor

@tsegismont tsegismont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @kdubb

kdubb added 2 commits August 2, 2023 18:25
This enables configuration of the new`maxLifetime` value provided by Vert.x 4.3 that ensures connections are recycled after a maximum duration.

Because `maxLifetime` could conceivable be set beyond the maximum value that milliseconds can store in an `int` (24.8 days) I created a `UnitisedTime` class. The `UnitisedTime` class has a `unitised` method that converts a `Duration` into the smallest possible `TimeUnit` that can be stored in an int, starting with milliseconds.

For consistency, I updated the `idleTimeout` to use the same `UnitisedTime.unitised` utitlity for conversion.
@gsmet gsmet force-pushed the feature/reactive-sql-max-lifetime branch from be79d01 to 7503e36 Compare August 2, 2023 16:25
@gsmet gsmet dismissed their stale review August 2, 2023 16:26

PR rebased.

@gsmet
Copy link
Member

gsmet commented Aug 2, 2023

@kdubb I rebased the PR on top of my other PR and made the necessary adjustments. I also got rid of the lambdas as we try to avoid lambdas in runtime code.
Let me know if you're still happy with it and we will merge.

@kdubb
Copy link
Contributor Author

kdubb commented Aug 2, 2023

@gsmet LGTM

@gsmet gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Aug 2, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Aug 2, 2023

Failing Jobs - Building 7503e36

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 17 Build Failures Logs Raw logs
✔️ JVM Tests - JDK 20

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 #

- Failing: integration-tests/kafka-oauth-keycloak 

📦 integration-tests/kafka-oauth-keycloak

io.quarkus.it.kafka.KafkaOauthTest.test - More details - Source on GitHub

java.lang.RuntimeException: java.lang.RuntimeException: Unable to start Quarkus test resource class io.quarkus.it.kafka.KafkaKeycloakTestResource
	at io.quarkus.test.junit.QuarkusTestExtension.throwBootFailureException(QuarkusTestExtension.java:639)
	at io.quarkus.test.junit.QuarkusTestExtension.interceptTestClassConstructor(QuarkusTestExtension.java:711)

@gsmet gsmet merged commit 3462ac0 into quarkusio:main Aug 8, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Aug 8, 2023
@quarkus-bot quarkus-bot bot added this to the 3.3 - main milestone Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants