Batch custom scopes activation #123

luca-bassoricci · 2022-05-16T15:02:36Z

Fixes Unable to activate batch custom scopes #122

Added job/step/partition scope management

I've added Quarkus specific implementation for batch scopes in runtime module and registered them in JBeretProcessor.
I marked PR as draft mainly because:

I'm not sure Quarkus specific implementations are implemented in the right way because they are reflection based
Tests are missing

radcortez · 2022-05-23T19:41:28Z

@luca-bassoricci Thanks for the PR.

Maybe we can implement some of the changes in JBeret directly (change method visibilities), so we can use them here directly without requiring reflection. I had to do some of that work when we first wrote the extension. I'm sure @chengfang will help.

If this is a major blocker for you, I'm fine if we move with the reflection based approach while we do the work in JBeret, but we do need tests :)

Thanks!

chengfang · 2022-05-23T20:02:28Z

@luca-bassoricci please file JBeret JIRA issues for changes needed from JBeret, and we'll evaluate if it's suitable to include it.

luca-bassoricci · 2022-05-25T06:57:27Z

I'm not in rush to resolve this issue (we're currently using job and step contexts to move data around job) but I'm doing this in my spare time, so in this period my time is limited.
In the meanwhile we can add some tests using current implementation and change it if something is changing in JBeret in the future, WDYT?

@luca-bassoricci Thanks for the PR.

Maybe we can implement some of the changes in JBeret directly (change method visibilities), so we can use them here directly without requiring reflection. I had to do some of that work when we first wrote the extension. I'm sure @chengfang will help.

If this is a major blocker for you, I'm fine if we move with the reflection based approach while we do the work in JBeret, but we do need tests :)

Thanks!

luca-bassoricci · 2022-05-25T07:20:49Z

@luca-bassoricci please file JBeret JIRA issues for changes needed from JBeret, and we'll evaluate if it's suitable to include it.

JIRA issue: https://issues.redhat.com/browse/JBERET-561

chengfang · 2022-05-26T21:39:54Z

@luca-bassoricci @radcortez all 3 ScopedContextImpl classes have a static INSTANCE field (though not public). Can you use them instead of accessing their constructor? If so, we can add a getInstance() public method in jberet ScopedContextImpl classes.

https://github.com/jberet/jsr352/blob/1.4.x/jberet-core/src/main/java/org/jberet/creation/JobScopedContextImpl.java#L24

chengfang · 2022-05-26T22:05:10Z

After reading more about this PR, I'd like to suggest something slightly different:

Can QuarkusJobScopedContextImpl and the like extend jberet's JobScopedContextImpl? This will avoid quite some method duplicatation, without the need to access the constructor.

Is the only purpose of accessing getJobScopedBeans is to use it in destroy() method? If so, the parent class in jberet can implement the destroy() method with the same logic accessing the jberet-private getJobScopedBeans(). So Quarkus side does not need to access getJobScopedBeans() at all.

Will the above work?

luca-bassoricci · 2022-05-27T08:51:00Z

@chengfang

all 3 ScopedContextImpl classes have a static INSTANCE field (though not public). Can you use them instead of accessing their constructor? If so, we can add a getInstance() public method in jberet ScopedContextImpl classes.

After reading more about this PR, I'd like to suggest something slightly different:

Can QuarkusJobScopedContextImpl and the like extend jberet's JobScopedContextImpl? This will avoid quite some method duplicatation, without the need to access the constructor.

As my personal flavor I prefer to access private INSTANCE fields via a static getInstance(); for example using a new class just for Quarkus (eg QuarkusCDIExtension) where scopes INSTANCEs are exposed using the a public getXXXScopeInstance(); this is because I prefer to use delegation over extension.

Is the only purpose of accessing getJobScopedBeans is to use it in destroy() method? If so, the parent class in jberet can implement the destroy() method with the same logic accessing the jberet-private getJobScopedBeans(). So Quarkus side does not need to access getJobScopedBeans() at all.

Will the above work?

Yep, for now the solely usage is that.
I have only some concerns about my implementation of AlterableContext#destroy([Contextual](https://docs.oracle.com/javaee/7/api/javax/enterprise/context/spi/Contextual.html)<?> contextual) but I opened a review on my PR to let @radcortez have a look and give his opinion about this problem.

chengfang · 2022-05-31T13:21:41Z

Okay, based on the above discussion, here is my plan for jberet-side changes:

add public static getInstance() method to jberet scoped context impl classes;
add public void destroy() method to jberet scoped context impl classes, so quarkus-jberet classes can either inherit or delegate to it.
constructor and getScopedBeans() methods in jberet will remain private (unchanged).

luca-bassoricci · 2022-05-31T14:36:11Z

For me is fine.
I'm not sure a single 'destroy()' method is enough in Jberet because Quarkus implementation is using a 'destroy(Contextual<>)', but I am waiting for @radcortez to get a look to my code and give us a feedback

chengfang · 2022-06-02T18:46:30Z

jberet/jsr352#232 is the jberet PR. Please review and let me know if anything else is needed.

Note that a single destroy(Contextual<?>) method is added. This method destroys a specific bean, but if called with null, it will behave the same as what you would want in destroy() -- to destroy all beans.

Also note that the latest quarkus InjectableContext interface has changed: there is now a destroy(ContextState) method replacing destroy(Contextual<?>).

luca-bassoricci · 2022-06-06T08:04:17Z

jberet/jsr352#232 is the jberet PR. Please review and let me know if anything else is needed.

Note that a single destroy(Contextual<?>) method is added. This method destroys a specific bean, but if called with null, it will behave the same as what you would want in destroy() -- to destroy all beans.

Also note that the latest quarkus InjectableContext interface has changed: there is now a destroy(ContextState) method replacing destroy(Contextual<?>).

I've seen your PR and LGTM, but I have also seen InjectableContext changes so I need more time to investigate.
I'll update you all ASAP.

luca-bassoricci · 2023-01-27T07:33:44Z

@chengfang

jberet/jsr352#232 is the jberet PR. Please review and let me know if anything else is needed.
Note that a single destroy(Contextual<?>) method is added. This method destroys a specific bean, but if called with null, it will behave the same as what you would want in destroy() -- to destroy all beans.
Also note that the latest quarkus InjectableContext interface has changed: there is now a destroy(ContextState) method replacing destroy(Contextual<?>).

I've seen your PR and LGTM, but I have also seen InjectableContext changes so I need more time to investigate. I'll update you all ASAP.

@chengfang
I suppose you can merge your PR.

chrisschauer · 2023-12-05T15:45:02Z

Any progress here? I stumbled over the same issue with JobScoped beans.
Thanks

radcortez · 2023-12-05T18:37:39Z

Unsure. This PR has been stale for some time. @luca-bassoricci do you intend to keep working on this? Thanks!

luca-bassoricci · 2023-12-06T06:52:47Z

I'll be happy to work on it but on my (short) spare time because I am a bit busy at work, sorry.
I pinged cheofang times ago but without response.
Codebase on my branch is still with Quarkus 2.x, so I need to upgrade to latest Quarkus and JBeret.
May we drop support for Quarkus 2.x, WDYT @radcortez ?

radcortez · 2023-12-06T12:45:24Z

May we drop support for Quarkus 2.x, WDYT @radcortez ?

Sure.

liweinan · 2023-12-06T15:06:46Z

I'll check this.

luca-bassoricci · 2023-12-07T09:18:16Z

I have aligned with latest extension codebase and JBeret 2.2.0-SNAPSHOT (current main) where chengfang PR will be merged later
@radcortez build fails because jberet 2.2.0-SNAPSHOT dependency: IDK which is the right way to proceed in this case when using an external snapshot repository as dep.
Should I ask liweinan to merge chengfang PR into main or we have to move in a different way?

radcortez · 2023-12-07T12:13:09Z

One technique that I use is to add a step into the ci builder to clone and build locally the snapshot dependency until we have a stable release.

luca-bassoricci · 2023-12-07T13:36:46Z

One technique that I use is to add a step into the ci builder to clone and build locally the snapshot dependency until we have a stable release.

That's fine.
Done.

luca-bassoricci · 2023-12-14T11:21:36Z

One technique that I use is to add a step into the ci builder to clone and build locally the snapshot dependency until we have a stable release.

That's fine. Done.

@radcortez I'm currently use the main branch of Jberet to build locally, but what about using chengfang:scope-impl-public-JBERET-561 instead? This should avoid @liweinan to merge the PR until we decide to remove the draft to this PR.
WDYT?

liweinan · 2023-12-14T16:56:55Z

.github/workflows/build.yml

+      - uses: actions/checkout@v4
+        name: checkout jberet snapshot
+        with:
+          repository: jberet/jsr352


@luca-bassoricci Maybe you can directly use cheng's fork repo here.

.github/workflows/build.yml

radcortez · 2023-12-15T10:13:41Z

@radcortez I'm currently use the main branch of Jberet to build locally, but what about using chengfang:scope-impl-public-JBERET-561 instead? This should avoid @liweinan to merge the PR until we decide to remove the draft to this PR. WDYT?

Yes, it's fine.

luca-bassoricci · 2023-12-15T10:44:48Z

@liweinan created a new branch based on jberet 2.x and I'm already using it; cheng's branch IMO can be dismissed because we don't support pre-jakarta code.
@liweinan no rush to merge your branch, we can use your branch until this PR is draft

liweinan · 2023-12-15T14:33:17Z

@luca-bassoricci Cool! :-) I'll finish doing TCK tests today but I guess do the release need sometime, anyway I'll finish working on this before next weekend(most probably before next Monday).

In addition, just to confirm that you don't need the PR for JBeret 1.4.x right? If that's true I'll just do the release for 2.1.x.

To be concise: If Quarkus side has no specific requirement, and if you can just use the jberet/jsr352#432 (comment), I'll do a release 2.2.0.Final from main branch, because branch 2.1 is for EAP8.

luca-bassoricci · 2023-12-15T15:21:20Z

In addition, just to confirm that you don't need the PR for JBeret 1.4.x right? If that's true I'll just do the release for 2.1.x.

Confirmed

To be concise: If Quarkus side has no specific requirement, and if you can just use the jberet/jsr352#432 (comment), I'll do a release 2.2.0.Final from main branch, because branch 2.1 is for EAP8.

IDK if Quarkus has specific requirements, maybe @radcortez can answer.
As stated above I'm currently use your PR branch while in draft but if backport to 2.1 (jberet/jsr352#432 (comment)) we can decide to move to 2.1.x-stable as base anytime

liweinan · 2023-12-15T15:25:36Z

@luca-bassoricci Thanks for the confirmation! Okay I'll do the 2.2.0.Final once TCK tests are done, and if it needs to be back-ported into branch 2.1 in the future please let me know.

liweinan · 2023-12-17T18:29:31Z

@luca-bassoricci I have released 2.2.0.Final for use: https://github.com/jberet/jsr352/releases/tag/2.2.0.Final If there is any problem please let me know. (The artifacts are uploaded to JBoss Nexus repo, it may needs sometime to sync to the Maven Central repo)

luca-bassoricci · 2023-12-20T16:00:11Z

If this is a major blocker for you, I'm fine if we move with the reflection based approach while we do the work in JBeret, but we do need tests :)

About tests:
My idea is to import JBeret tests from https://github.com/jberet/jsr352/tree/main/test-apps/cdiScopes and add them to integration-test module instead of create a new set of tests from scratch.

@radcortez WDYT?

radcortez · 2023-12-22T12:34:41Z

About tests: My idea is to import JBeret tests from https://github.com/jberet/jsr352/tree/main/test-apps/cdiScopes and add them to integration-test module instead of create a new set of tests from scratch.

@radcortez WDYT?

Sounds good.

luca-bassoricci · 2023-12-22T23:20:15Z

Tests added.
I have used JBeret test artifacts, create alternative beans because JBeret's ones are not @Dependent and copied *IT tests.
commons project contains a base abstract test class useful to integrate with JBeret test classes.
I had to add a reflection call here but I don't think is a concern because it only a test and not runtime code.
Let me know.

luca-bassoricci · 2024-01-10T08:50:54Z

Tests added. I have used JBeret test artifacts, create alternative beans because JBeret's ones are not @Dependent and copied *IT tests. commons project contains a base abstract test class useful to integrate with JBeret test classes. I had to add a reflection call here but I don't think is a concern because it only a test and not runtime code. Let me know.

@radcortez any comment?

radcortez · 2024-01-10T17:02:35Z

@radcortez any comment?

Yes, it is fine. Thank you.

So, is this complete? Or are we still missing something?

luca-bassoricci · 2024-01-12T06:53:04Z

@radcortez any comment?

Yes, it is fine. Thank you.

So, is this complete? Or are we still missing something?

From my POV all seems to be fine; for me a big concern was about the tests but after JBeret's tests passed I'm more confident.
I suppose draft status can be removed and PR merged.

liweinan · 2024-01-12T07:57:34Z

btw JBeret 2.2.0.Final has been merged into WildFly main branch: wildfly/wildfly#17519 (comment)

radcortez · 2024-01-15T11:18:41Z

core/deployment/src/main/java/io/quarkiverse/jberet/deployment/JBeretProcessor.java

@@ -99,28 +110,37 @@ public class JBeretProcessor {
    private static final DotName JOB = DotName.createSimple(Job.class);

    @BuildStep
-    public void registerExtension(BuildProducer<FeatureBuildItem> feature, BuildProducer<CapabilityBuildItem> capability) {
+    public void registerExtension(BuildProducer<FeatureBuildItem> feature,


It seems that in this class, there are a bunch of changes that are only code style changes and not related to the actual feature. Any chance we can revert these and keep only the relevant changes?

Added job/step/partition scope management Add IT tests copied from https://github.com/jberet/jsr352/tree/main/test-apps/cdiScopes

radcortez

Thank you for all the work. Really appreciate it :)

luca-bassoricci mentioned this pull request May 16, 2022

Unable to activate batch custom scopes #122

Closed

chengfang mentioned this pull request Jun 1, 2022

JBERET-561 Change visibility of scopes implementations jberet/jsr352#232

Closed

luca-bassoricci force-pushed the issue-122 branch from 6a03497 to 6c5eb95 Compare December 7, 2023 09:06

luca-bassoricci force-pushed the issue-122 branch from 6c5eb95 to 3e924f0 Compare December 9, 2023 18:09

liweinan reviewed Dec 14, 2023

View reviewed changes

.github/workflows/build.yml Outdated Show resolved Hide resolved

liweinan mentioned this pull request Dec 15, 2023

JBERET-561 Change visibility of scopes implementations jberet/jsr352#432

Merged

luca-bassoricci force-pushed the issue-122 branch from 26a0f6c to 0f976d5 Compare December 18, 2023 07:16

luca-bassoricci force-pushed the issue-122 branch from 0f976d5 to 4a7a042 Compare December 22, 2023 23:15

radcortez marked this pull request as ready for review January 12, 2024 09:11

luca-bassoricci force-pushed the issue-122 branch from 4a7a042 to 4a6ce36 Compare January 14, 2024 19:10

radcortez requested changes Jan 15, 2024

View reviewed changes

luca-bassoricci force-pushed the issue-122 branch from 4a6ce36 to e97e5e6 Compare January 15, 2024 15:42

luca-bassoricci requested a review from radcortez January 15, 2024 15:43

Batch custom scopes activation

9b06368

Added job/step/partition scope management Add IT tests copied from https://github.com/jberet/jsr352/tree/main/test-apps/cdiScopes

radcortez force-pushed the issue-122 branch from e97e5e6 to 9b06368 Compare January 15, 2024 21:32

radcortez approved these changes Jan 16, 2024

View reviewed changes

radcortez merged commit 5331792 into quarkiverse:main Jan 16, 2024
1 check passed

radcortez added this to the 2.3.0 milestone Jan 16, 2024

luca-bassoricci mentioned this pull request Jan 29, 2024

Support @StepScoped / @JobScoped #285

Closed

Batch custom scopes activation #123

Batch custom scopes activation #123

Conversation

luca-bassoricci commented May 16, 2022 • edited by radcortez

radcortez commented May 23, 2022

chengfang commented May 23, 2022

luca-bassoricci commented May 25, 2022

luca-bassoricci commented May 25, 2022

chengfang commented May 26, 2022 • edited

chengfang commented May 26, 2022

luca-bassoricci commented May 27, 2022 • edited

chengfang commented May 31, 2022

luca-bassoricci commented May 31, 2022

chengfang commented Jun 2, 2022

luca-bassoricci commented Jun 6, 2022

luca-bassoricci commented Jan 27, 2023

chrisschauer commented Dec 5, 2023

radcortez commented Dec 5, 2023

luca-bassoricci commented Dec 6, 2023

radcortez commented Dec 6, 2023

liweinan commented Dec 6, 2023

luca-bassoricci commented Dec 7, 2023 • edited

radcortez commented Dec 7, 2023

luca-bassoricci commented Dec 7, 2023 • edited

luca-bassoricci commented Dec 14, 2023

liweinan Dec 14, 2023

Choose a reason for hiding this comment

radcortez commented Dec 15, 2023

luca-bassoricci commented Dec 15, 2023

liweinan commented Dec 15, 2023 • edited

luca-bassoricci commented Dec 15, 2023

liweinan commented Dec 15, 2023

liweinan commented Dec 17, 2023 • edited

luca-bassoricci commented Dec 20, 2023

radcortez commented Dec 22, 2023

luca-bassoricci commented Dec 22, 2023 • edited

luca-bassoricci commented Jan 10, 2024

radcortez commented Jan 10, 2024

luca-bassoricci commented Jan 12, 2024

liweinan commented Jan 12, 2024

radcortez Jan 15, 2024

Choose a reason for hiding this comment

radcortez left a comment

Choose a reason for hiding this comment

luca-bassoricci commented May 16, 2022 •

edited by radcortez

chengfang commented May 26, 2022 •

edited

luca-bassoricci commented May 27, 2022 •

edited

luca-bassoricci commented Dec 7, 2023 •

edited

luca-bassoricci commented Dec 7, 2023 •

edited

liweinan commented Dec 15, 2023 •

edited

liweinan commented Dec 17, 2023 •

edited

luca-bassoricci commented Dec 22, 2023 •

edited