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

Move REST Client configuration to use @ConfigMapping #42106

Merged
merged 2 commits into from
Aug 1, 2024

Conversation

radcortez
Copy link
Member

@radcortez radcortez commented Jul 24, 2024

I've tried to be somehow conservative with the changes (if you can call this PR that :))

The old REST Client configuration performed look-ups to all acceptable names and merged the final configuration. The standard Config system does not support this (and other extensions don't either). It was specifically implemented with the old behaviour, which is also supported in the new implementation via fallbacks, except for a minor breaking change: simple names REST Client names quoted cannot be mixed with unquoted and FQN. This can easily be added if required with another fallback.

@geoand
Copy link
Contributor

geoand commented Jul 24, 2024

This will conflict head-on with #42100 :)

@radcortez
Copy link
Member Author

This will conflict head-on with #42100 :)

Ops... this was because of #41948 (comment). This should also help with the config overhead. The mapping will load and cache the config at startup time.

Sorry for the extra work :(

@gsmet
Copy link
Member

gsmet commented Jul 24, 2024

Yeah, so, I won't fight this one as already done but let's not start a massive rewrite of extensions to @ConfigMapping because that makes things a moving target for me. Thanks!

@geoand
Copy link
Contributor

geoand commented Jul 24, 2024

Sorry for the extra work :(

No problem. If we can get something better for REST config, I'm all for it because it's a real pain to maintain

@radcortez
Copy link
Member Author

Yeah, so, I won't fight this one as already done but let's not start a massive rewrite of extensions to @ConfigMapping because that makes things a moving target for me. Thanks!

The intent was not to start a massive rewrite of extensions but to fix only the REST Client config since it neither uses the old @ConfigRoot class implementation nor @ConfigMapping, but a custom-based implementation with CDI dependencies.

One annoying issue is this:

And I guess #42092, which I overlooked but would also help.

@radcortez radcortez marked this pull request as ready for review July 24, 2024 13:13
@quarkusio quarkusio deleted a comment from quarkus-bot bot Jul 24, 2024
@gsmet
Copy link
Member

gsmet commented Jul 24, 2024

Yeah, I understand that, I just wanted it to be clear :).

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Assuming CI passes, this is great!

This comment has been minimized.

@radcortez radcortez force-pushed the rest-client-config branch from 08d2e3a to 2924a44 Compare July 24, 2024 16:18

This comment has been minimized.

@gsmet
Copy link
Member

gsmet commented Jul 24, 2024

At least some of the failures look related.

@radcortez radcortez force-pushed the rest-client-config branch from 2924a44 to e1a9eae Compare July 24, 2024 21:20

This comment has been minimized.

This comment has been minimized.

@geoand
Copy link
Contributor

geoand commented Jul 25, 2024

The test failure seems related, but it's probably easily fixable

@radcortez
Copy link
Member Author

The problem here is how we handle the test url properties. There is some sanitization code to remove the ending slash and rewrite the configuration as a system property.

Some pieces of the code may see different configuration values depending on when they are executed. In this case, the REST Client configuration was only queried when the REST Client was created (after the test URL was sanitized). Now, it is created at startup (before the test URL was sanitized). For the REST Client execution, the sanitization doesn't matter, but the tracing code asserted the client URL without an ending slash, which was removed by the sanitization.

I've added a separate commit to handle the sanitization at the config level, so all executions should see the same values.

As a separate discussion, we must consider if changing the configuration after startup and rewriting the system properties is the proper behaviour. It is convenient for sure, and we haven't had many issues around it, but I think it is a problem. See also #41957.

@geoand
Copy link
Contributor

geoand commented Jul 25, 2024

As a separate discussion, we must consider if changing the configuration after startup and rewriting the system properties is the proper behaviour.

We are doing this?

@radcortez
Copy link
Member Author

We are doing this?

Yes. Here are a couple of examples:

I'm unsure if this is only required for testing or if we have cases where we rely on system properties changed in the prod profile. I'll have to analyze it.

@geoand
Copy link
Contributor

geoand commented Jul 25, 2024

Okay, those are the ones I had in mind

Copy link

quarkus-bot bot commented Jul 25, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 044af63.

Failing Jobs

Status Name Step Failures Logs Raw logs Build scan
✔️ JVM Tests - JDK 17 Build Logs Raw logs 🔍
✔️ JVM Tests - JDK 21 Build Logs Raw logs 🔍
JVM Tests - JDK 17 Windows Build Failures Logs Raw logs 🔍

Full information is available in the Build summary check run.
You can consult the Develocity build scans.

Failures

⚙️ JVM Tests - JDK 17 Windows #

- Failing: extensions/smallrye-reactive-messaging-amqp/deployment 
! Skipped: integration-tests/reactive-messaging-amqp integration-tests/virtual-threads/amqp-virtual-threads 

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

io.quarkus.smallrye.reactivemessaging.amqp.AnonymousAmqpTest.test line 30 - History - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.smallrye.reactivemessaging.amqp.AnonymousAmqpTest was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at io.quarkus.smallrye.reactivemessaging.amqp.AnonymousAmqpTest.test(AnonymousAmqpTest.java:30)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)

io.quarkus.smallrye.reactivemessaging.amqp.SecuredAmqpTest.test line 28 - History - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.smallrye.reactivemessaging.amqp.SecuredAmqpTest was not fulfilled within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at io.quarkus.smallrye.reactivemessaging.amqp.SecuredAmqpTest.test(SecuredAmqpTest.java:28)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)

io.quarkus.smallrye.reactivemessaging.amqp.devmode.AmqpDevModeTest.testCodeUpdate line 44 - History - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.smallrye.reactivemessaging.amqp.devmode.AmqpDevModeTest was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at io.quarkus.smallrye.reactivemessaging.amqp.devmode.AmqpDevModeTest.testCodeUpdate(AmqpDevModeTest.java:44)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)

io.quarkus.smallrye.reactivemessaging.amqp.devmode.nohttp.AmqpDevModeNoHttpTest.testConsumerUpdate line 77 - History - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.amqp.devmode.nohttp.AmqpDevModeNoHttpTest 
Expecting size of:
  []
to be greater than or equal to 5 but was 0 within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

io.quarkus.smallrye.reactivemessaging.amqp.devmode.nohttp.AmqpDevModeNoHttpTest.testProducerUpdate line 48 - History - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.amqp.devmode.nohttp.AmqpDevModeNoHttpTest 
Expecting size of:
  []
to be greater than or equal to 5 but was 0 within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 integration-tests/opentelemetry

io.quarkus.it.opentelemetry.OpenTelemetryInjectionsTest.testOTelInjections - History

  • Condition with Lambda expression in io.quarkus.it.opentelemetry.OpenTelemetryInjectionsTest was not fulfilled within 5 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.it.opentelemetry.OpenTelemetryInjectionsTest was not fulfilled within 5 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at io.quarkus.it.opentelemetry.OpenTelemetryInjectionsTest.reset(OpenTelemetryInjectionsTest.java:26)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)

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.

None yet

3 participants