-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Removed Panache marker and annotation processor in favour of Jandex use-site indexing #38242
Conversation
This comment has been minimized.
This comment has been minimized.
Well, it was worth a try :( I'm not sure what went wrong though, it should work, minus the removal of producing @aloubyansky I see your name in the commits of |
In particular, this change appears to not break the ORM/Panache tests, but does break the HR/Panache tests, where they fail to get a transaction. I really don't think this is due to improper entity instrumentation. It could be the application-archive marker change, perhaps. |
This stack trace appears to indicate that |
@mkouba do you know how I could check why the interceptor is not being applied? |
Actually, it's weird, because it turns out that @BuildStep
void registerInterceptors(BuildProducer<AdditionalBeanBuildItem> additionalBeans) {
AdditionalBeanBuildItem.Builder builder = AdditionalBeanBuildItem.builder();
builder.addBeanClass(WithSessionOnDemandInterceptor.class);
builder.addBeanClass(WithSessionInterceptor.class);
builder.addBeanClass(ReactiveTransactionalInterceptor.class);
// this was missing
builder.addBeanClass(WithTransactionInterceptor.class);
additionalBeans.produce(builder.build());
} But now, the mystery is: how could it possibly have worked before I added this? |
"Application archives" are indexed, while the rest of the classpath isn't. |
This comment has been minimized.
This comment has been minimized.
OK, so, before, we used to mark jars having Panache in their classpath as requiring indexing, so we could find the users of Panache, to instrument them. Now, I want to use Jandex to find users of the entities, but for that I need to find the entities, which means I need jars containing entities to be indexed. If they're not indexed, this won't work. And yet, Hibernate ORM also finds entities using the index, and they manage to do it without having any marker. Well, not true, they say you need a So, we could have the same requirement, instead of requiring an APT processor, and it should Just Work™. |
CI failure looks totally unrelated, let's rebase and try again. |
7657d33
to
20465d8
Compare
I think that it worked before because the |
😂😂😂😂😂😂😂 That's such an incredible side-effect, but indeed it makes sense, you cracked it! Wow! |
This comment has been minimized.
This comment has been minimized.
I think we need the flag file |
#38282 is an implementation of the above suggestion |
Try rebasing onto |
Thanks! |
…se-site indexing We now use Jandex use-site indexing to find classes to process, it should be faster than Panache markers set by the annotation processor because we only find classes using the entities, and not instrument the entire jars.
…nsaction` interceptor
20465d8
to
c40d6da
Compare
This comment has been minimized.
This comment has been minimized.
I'm going to assume this can't possibly be related to anything and restart this job. |
Yeah, that should be a random failure |
This comment has been minimized.
This comment has been minimized.
Not convinced by those test failures either :( |
Likely unrelated, but having a fresh run wouldn't hurt |
Yeah, I restarted the failed runs. |
✔️ 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. |
Looks like it runs. Let's merge it and I'll update the docs in a new PR, to save on a rebuild. |
Docs at #38327 |
Based on: quarkusio#38242 (comment) (cherry picked from commit 341a713)
We now use Jandex use-site indexing to find classes to process, it should be faster than Panache markers set by the annotation processor because we only find classes using the entities, and not instrument the entire jars.
Let's see how much CI I break with this change 😱