Document default connection-pool-size for REST Clients and raise default pool size to 50 for Quarkus REST

[weltonrodrigo]

Default value for quarkus.rest-client.connection-pool-size is 50. This is the default value set by Resteasy itself that quarkus don't override by default: https://github.com/resteasy/resteasy/blob/0ccb17086c7dfffd0d6a7970db9b147fa7a0fb5c/resteasy-client/src/main/java/org/jboss/resteasy/client/jaxrs/internal/ResteasyClientBuilderImpl.java#L62

[gsmet]

Unlike its position in the tree could indicate, I think this config is used for both the classic and the reactive REST Clients. So we would need to make sure this change is correct for both of them.

[weltonrodrigo]

Unlike its position in the tree could indicate, I think this config is used for both the classic and the reactive REST Clients. So we would need to make sure this change is correct for both of them.

I managed to find that the default for the resteasty-reactive was 20:

quarkus/independent-projects/resteasy-reactive/client/runtime/src/main/java/org/jboss/resteasy/reactive/client/impl/ClientImpl.java

Line 92 in 35ccb2e

private static final int DEFAULT_CONNECTION_POOL_SIZE = 20;

I managed to confirm that indeed the client is created with 20 for older quakus versions when the using

  <dependency>
      <groupId>io.quarkus</groupId>
      <artifactId>quarkus-rest-client-reactive</artifactId>
     <version>3.6.3</version>
  </dependency>

But I must admit I'm quite lost here about what do you mean by both classic and reactive REST clients. Isn't the reactive client the default and isn't the reactive the client used when following this guide https://quarkus.io/guides/rest-client ?

If you can gimme a pom that will use the classic rest I can check what is the actual value.

[gsmet]

Sorry, I forgot about this one. My point is that while you are keeping the same default for RESTEasy Classic, you are actually changing the default for RESTEasy Reactive - given they use the same config (except if I'm mistaken?).

I'm fine having the same default but we need to make sure it makes sense.

50 looks like a lot to me and I would favor 20 for both but I don't know :)

[gsmet]

@weltonrodrigo I thought about it a bit more and I think you're right that we should align with 50 as the default value. I tried to be explicit in the commit comment:

The default REST Client connection pool size for RESTEasy Classic was
50, but the one for Quarkus REST is 20.
Given most of the time, the limiting factor will be the responsiveness of the
server, not the speed of the implementation, I think we should be
conservative and keep the same values.

@geoand do you agree with this analysis?

[geoand]

Yeah, that makes sense

[github-actions]

🎊 PR Preview aa638a1 has been successfully built and deployed to https://quarkus-pr-main-45148-preview.surge.sh/version/main/guides/

• Images of blog posts older than 3 months are not available.
• Newsletters older than 3 months are not available.

[weltonrodrigo]

@weltonrodrigo I thought about it a bit more and I think you're right that we should align with 50 as the default value. I tried to be explicit in the commit comment:

I believe you'll find this funny, but I actually though @WithDefault("50") would just document this value as being 50 in the generated documentation pages. I now understand that this actually sets the value to 50 when no user provided config is present.

This is funny because I was both wrong and right. Wrong because I wrongly interpreted the code and misunderstood the value to be 50 and right because it seems it actually should be 50.

And now this thread makes so much sense to me now.

[gsmet]

I believe you'll find this funny, but I actually though @WithDefault("50") would just document this value as being 50 in the generated documentation pages. I now understand that this actually sets the value to 50 when no user provided config is present.

This is actually what is required for the test to pass so I switched to an annotation to only document the behavior.

Still it's better to have everything aligned between RESTEasy and Quarkus REST.

I wonder if we should go through the other properties and check how aligned they are and document their values. It's a bit hard to figure out the defaults atm.

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 56c2c32.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 56c2c32.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

---

Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-reactive-messaging-kafka/deployment

✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.testing.KafkaDevServicesContinuousTestingWorkingAppPropsTestCase.testContinuousTestingScenario3 - History

• io.quarkus.builder.BuildException: Build failure: Build failed due to errors [error]: Build step io.quarkus.redis.deployment.client.DevServicesRedisProcessor\#startRedisContainers threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image docker.io/redis:7 at io.quarkus.redis.deployment.client.DevServicesRedisProcessor.startRedisContainers(DevServicesRedisProcessor.java:124) at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:733) at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:856) - java.lang.RuntimeException

java.lang.RuntimeException: 
io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.redis.deployment.client.DevServicesRedisProcessor#startRedisContainers threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image docker.io/redis:7
	at io.quarkus.redis.deployment.client.DevServicesRedisProcessor.startRedisContainers(DevServicesRedisProcessor.java:124)
	at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:733)
	at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:856)
	at io.quarkus.builder.BuildContext.run(BuildContext.java:255)
	at org.jboss.threads.ContextHandler$1.runWith(ContextHandler.java:18)