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

ArC: initial implementation of CDI Build Compatible Extensions #31444

Merged
merged 2 commits into from
Mar 3, 2023

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Feb 27, 2023

Related to #28558

Vast majority of changes are additions that are pretty well isolated. To review, it's probably best to focus on changes to already existing files.

I plan to submit another PR with additions of the CDI TCK runner modules, as this PR is huge already.

@Ladicek Ladicek requested review from mkouba and manovotn February 27, 2023 11:56
@quarkus-bot quarkus-bot bot added the area/arc Issue related to ARC (dependency injection) label Feb 27, 2023
@@ -987,8 +987,13 @@ Builder disposer(DisposerInfo disposer) {
return this;
}

/**
* @deprecated use {@link #alternative(boolean)} and {@link #priority(Integer)};
* this method will be removed at some time after Quarkus 3.6
Copy link
Contributor

Choose a reason for hiding this comment

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

Quarkus 3.6??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking roughly 6 months after the Quarkus release in which this would happen, which is 3.0. Given our roughly monthly release cadence, that's 3.6. Do you have another preference?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say that Deprecated.forRemoval()=true is enough - BeanInfo.Builder.alternativePriority(Integer) is not part of the public API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added @Deprecated(forRemoval = true, since = "3.0").

@@ -201,12 +201,22 @@ public THIS targetPackageName(String name) {
return self();
}

/**
* @deprecated use {@link #alternative(boolean)} and {@link #priority(int)};
* this method will be removed at some time after Quarkus 3.6
Copy link
Member

Choose a reason for hiding this comment

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

I would recommend to rather use since = "3.0" in the @Deprecated annotation.

We never really give estimate of when something will be removed. But us knowing when it was removed is useful for knowing when it's relatively safe to remove something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We actually often do that in ArC, but adding since is a great point, I'll do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for since

Copy link
Contributor Author

@Ladicek Ladicek Feb 27, 2023

Choose a reason for hiding this comment

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

Added @Deprecated(forRemoval = true, since = "3.0").

@Ladicek Ladicek force-pushed the arc-build-compatible-extensions branch from e98a374 to 75a8e79 Compare February 27, 2023 14:10
@Ladicek
Copy link
Contributor Author

Ladicek commented Feb 27, 2023

Added @Deprecated(forRemoval = true, since = "3.0") to the elements deprecated in this PR, plus created another commit to add the same to elements that were deprecated for removal recently.

@quarkus-bot

This comment has been minimized.

@Ladicek Ladicek force-pushed the arc-build-compatible-extensions branch from 75a8e79 to 8c7c1ce Compare February 28, 2023 14:09
@Ladicek
Copy link
Contributor Author

Ladicek commented Feb 28, 2023

Made the changes we discussed earlier today. One thing I forgot: I'd appreciate a second pair of eyes on the changes to InstanceImpl. We can discuss that offline, but there are 2 main changes:

  1. Add a way to construct an Instance<Object> for the @Synthesis phase, which prevents looking up InjectionPoint for non-@Dependent beans.
  2. Added ability to disable resetting the "current" injection point, which is also required for @Synthesis.

Maybe I should replace this code with synthetic injection points, as introduced recently. That might simplify things a lot, though I'm not sure it fully implements all the peculiarities of looking up a "current" injection point.

@quarkus-bot

This comment has been minimized.

@mkouba
Copy link
Contributor

mkouba commented Mar 1, 2023

Maybe I should replace this code with synthetic injection points, as introduced recently.

+1

That might simplify things a lot, though I'm not sure it fully implements all the peculiarities of looking up a "current" injection point.

What peculiarities do you have in mind? You can take a look at the SyntheticInjectionPointMetadataTest and SyntheticInjectionPointMetadataInvalidTest to see what's tested (hence supported ;-).

@Ladicek Ladicek force-pushed the arc-build-compatible-extensions branch from 8c7c1ce to 50d4034 Compare March 1, 2023 11:56
@Ladicek
Copy link
Contributor Author

Ladicek commented Mar 1, 2023

Rebased to avoid the failures.

After offline discussion, I left InstanceImpl.forSynthesis() in place instead of replacing it with a synthetic injection point of type Instance<Object>, because that would effectively turn off unused bean removal in presence of any BCExtension that creates a synthetic bean.

@quarkus-bot

This comment has been minimized.

@quarkus-bot
Copy link

quarkus-bot bot commented Mar 2, 2023

Failing Jobs - Building 50d4034

Status Name Step Failures Logs Raw logs
Native Tests - Misc4 Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Native Tests - Misc4 #

- Failing: integration-tests/opentelemetry-jdbc-instrumentation 

📦 integration-tests/opentelemetry-jdbc-instrumentation

io.quarkus.it.opentelemetry.PostgresOpenTelemetryJdbcInstrumentationIT.testPostgreSqlQueryTraced - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code <200> but was <500>.

@Ladicek
Copy link
Contributor Author

Ladicek commented Mar 3, 2023

The PostgresOpenTelemetryJdbcInstrumentationIT.testPostgreSqlQueryTraced fails because of

2023-03-02 16:15:57,122 ERROR [io.qua.ver.htt.run.QuarkusErrorHandler] (executor-thread-0) HTTP Request to /hit/postgresql?id=1 failed, error id: 62fd67f1-03fe-40ac-a2fc-8bd3e5373a22-1:
java.lang.IllegalStateException: Cannot retrieve the EntityManagerFactory/SessionFactory for persistence unit postgresql:
Hibernate ORM was deactivated through configuration properties

The exact same failure seems to occur on a bunch of other PRs: https://github.com/quarkusio/quarkus/pulls?q=is%3Apr+PostgresOpenTelemetryJdbcInstrumentationIT+testPostgreSqlQueryTraced

Gonna merge this.

@Ladicek Ladicek merged commit 2477362 into quarkusio:main Mar 3, 2023
@quarkus-bot quarkus-bot bot added this to the 3.0 - main milestone Mar 3, 2023
@Ladicek Ladicek deleted the arc-build-compatible-extensions branch March 3, 2023 12:36
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants