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 - introduce immutable bean archive index #29641

Merged
merged 1 commit into from Dec 5, 2022

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Dec 2, 2022

@quarkus-bot quarkus-bot bot added the area/arc Issue related to ARC (dependency injection) label Dec 2, 2022
@mkouba mkouba added triage/backport? area/arc Issue related to ARC (dependency injection) and removed area/arc Issue related to ARC (dependency injection) labels Dec 2, 2022
@Ladicek
Copy link
Contributor

Ladicek commented Dec 2, 2022

Interesting approach, but makes sense. I guess the intent is that the computing index should be used most of the time (or fallback to the immutable one if the computing doesn't exist), but there are certain occasions where the immutable index must be used unconditionally? It would be nice to have a little more detailed explanation of when the 2nd condition occurs.

Aside: I so wish we had a complete index of everything (both the Quarkus combined index and the ArC bean archive index) so that we don't have to do on-demand indexing at all...

@quarkus-bot

This comment has been minimized.

@mkouba
Copy link
Contributor Author

mkouba commented Dec 2, 2022

It would be nice to have a little more detailed explanation of when the 2nd condition occurs.

Yes, well it's described here and here but I can try to add more description to the BeanArchiveIndexBuildItem.

Aside: I so wish we had a complete index of everything (both the Quarkus combined index and the ArC bean archive index) so that we don't have to do on-demand indexing at all...

That would be nice but in that case we would have index the whole classpath (including jdk classes) which is probably unrealistic.

@Ladicek
Copy link
Contributor

Ladicek commented Dec 5, 2022

It would be nice to have a little more detailed explanation of when the 2nd condition occurs.

Yes, well it's described here and here but I can try to add more description to the BeanArchiveIndexBuildItem.

Those are exactly the descriptions that should be more detailed in my opinion. I mean, as ArC contributor, how do I know which index should I use? The optional one most of the time, probably, but I'm not really sure :-)

Aside: I so wish we had a complete index of everything (both the Quarkus combined index and the ArC bean archive index) so that we don't have to do on-demand indexing at all...

That would be nice but in that case we would have index the whole classpath (including jdk classes) which is probably unrealistic.

It wouldn't have to be the entire JDK, we'd just have to index the transitive closure of the application classes. I didn't try, so I don't know how crazy that would eventually become :-)

@mkouba
Copy link
Contributor Author

mkouba commented Dec 5, 2022

Those are exactly the descriptions that should be more detailed in my opinion. I mean, as ArC contributor, how do I know which index should I use? The optional one most of the time, probably, but I'm not really sure :-)

And my answer would be "it depends" :-). In general, you should use the immutable index to discover the components (beans, observers, etc.) - that's the case we're trying to fix with this PR - and the computing index for any other stuff. That's why we return the computing index from the BeanDeployment.getBeanArchiveIndex().

We should also clarify that the computing index is built on top of the immutable index, i.e. only classes that are not part of the immutable index can be computed.

@Ladicek
Copy link
Contributor

Ladicek commented Dec 5, 2022

Okay, so basically type discovery should use the immutable index, and bean discovery should use the computing index? Or something like that.

We should also clarify that the computing index is built on top of the immutable index, i.e. only classes that are not part of the immutable index can be computed.

Great point. The BeanProcessor builder currently has 2 setters, so we rely on its caller to maintain this invariant. I guess we can't really build the computing index internally and have to obtain it from the outside?

@mkouba
Copy link
Contributor Author

mkouba commented Dec 5, 2022

Great point. The BeanProcessor builder currently has 2 setters, so we rely on its caller to maintain this invariant. I guess we can't really build the computing index internally and have to obtain it from the outside?

Hm, in theory we could but I'd like to keep it more flexible for now. And given the fact that only the arc extension and ArcTestContainer are the "callers" I think that it's ok.

@mkouba mkouba requested a review from Ladicek December 5, 2022 10:41
@quarkus-bot

This comment has been minimized.

@quarkus-bot quarkus-bot bot added the area/spring Issues relating to the Spring integration label Dec 5, 2022
- this is index is mandatory and is used to discover beans
- the computing index is optional and can be used for other tasks, e.g. during type-safe resolution
- if the computing index is not present the immutable index is used
instead
- fixes quarkusio#29575
@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Dec 5, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Dec 5, 2022

Failing Jobs - Building c2ab342

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 11 Windows Build Failures Logs Raw logs
✔️ JVM Tests - JDK 17
JVM Tests - JDK 17 MacOS M1 Set up runner ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 18

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 Windows #

- Failing: extensions/quartz/deployment 
! Skipped: integration-tests/quartz 

📦 extensions/quartz/deployment

io.quarkus.quartz.test.FireTimeTest.testExecution line 39 - More details - Source on GitHub

java.lang.AssertionError: 

Expecting actual:

@gsmet gsmet merged commit 2789dc7 into quarkusio:main Dec 5, 2022
@quarkus-bot quarkus-bot bot added this to the 2.16 - main milestone Dec 5, 2022
@quarkus-bot quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Dec 5, 2022
@gsmet gsmet modified the milestones: 2.16 - main, 2.15.0.Final Dec 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/spring Issues relating to the Spring integration kind/bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected beans added to the index and affecting restarting the application
5 participants