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

SmallRye CP - introduce ThreadContextProviderBuildItem #27685

Merged
merged 2 commits into from
Sep 6, 2022

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Sep 2, 2022

This PR introduces the ThreadContextProviderBuildItem so that it's possible to register a CP ThreadContextProvider via build item (in addition to the current service loader facility). Furhtermore, a new ArC config property is introduced - quarkus.arc.context-propagation.enabled. This property is a temporary solution, i.e. the property itself could be removed in the future or the default value may be subject of change.

@mkouba mkouba requested a review from manovotn September 2, 2022 08:28
@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/context-propagation area/dependencies Pull requests that update a dependency file area/smallrye labels Sep 2, 2022
Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

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

Generally looks good, I have two notes though
Firstly, this has the downside that I mentioned here. I am not sure that's something that has been considered and is acceptable?
Secondly, we should have some test for it; you can take a look at these tests for inspiration and do a similar one with your config turned off.

@mkouba
Copy link
Contributor Author

mkouba commented Sep 2, 2022

Secondly, we should have some test for it; you can take a look at these tests for inspiration and do a similar one with your config turned off.

+1

@mkouba mkouba requested review from manovotn and Ladicek September 5, 2022 08:22
@mkouba mkouba force-pushed the cp-thread-context-provider-build-item branch from 7675cae to bf375bf Compare September 5, 2022 08:22
@quarkus-bot

This comment has been minimized.

@Ladicek
Copy link
Contributor

Ladicek commented Sep 5, 2022

Otherwise LGTM.

@mkouba mkouba force-pushed the cp-thread-context-provider-build-item branch from bf375bf to 4d8ecb8 Compare September 5, 2022 09:29
@quarkus-bot

This comment has been minimized.

@Ladicek
Copy link
Contributor

Ladicek commented Sep 5, 2022

Probably have to move the tests into the SRye ConProp extension?

@mkouba mkouba force-pushed the cp-thread-context-provider-build-item branch from 4d8ecb8 to 2bc2b52 Compare September 5, 2022 10:22
@mkouba mkouba requested a review from Ladicek September 5, 2022 10:22
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@mkouba
Copy link
Contributor Author

mkouba commented Sep 5, 2022

The main is broken. We'll have to wait for the fix..

@mkouba mkouba force-pushed the cp-thread-context-provider-build-item branch from 2bc2b52 to be0322e Compare September 5, 2022 13:14
@quarkus-bot
Copy link

quarkus-bot bot commented Sep 5, 2022

Failing Jobs - Building be0322e

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

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: core/deployment 
! Skipped: devtools/cli devtools/gradle/gradle-application-plugin devtools/gradle/gradle-extension-plugin and 644 more

📦 core/deployment

io.quarkus.deployment.dev.FileSystemWatcherTestCase.testFileSystemWatcher line 121 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: expected: not <null>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:152)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)

⚙️ JVM Tests - JDK 11 Windows #

- Failing: extensions/opentelemetry/opentelemetry/deployment 
! Skipped: extensions/opentelemetry/opentelemetry-exporter-jaeger/deployment extensions/opentelemetry/opentelemetry-exporter-otlp/deployment integration-tests/micrometer-prometheus and 6 more

📦 extensions/opentelemetry/opentelemetry/deployment

io.quarkus.opentelemetry.deployment.RestClientOpenTelemetryTest.urlWithoutAuthentication line 111 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: expected: <CLIENT> but was: <SERVER>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)

@mkouba mkouba merged commit 6ba12ab into quarkusio:main Sep 6, 2022
@quarkus-bot quarkus-bot bot added this to the 2.13 - main milestone Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) area/context-propagation area/dependencies Pull requests that update a dependency file area/smallrye
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants