search

jakartaee / batch

The Jakarta Batch project produces the Batch Specification and API.
https://projects.eclipse.org/projects/ee4j.batch
Apache License 2.0
13 stars 17 forks source link

Add spec requirement for JobOperator CDI Bean #183

Closed scottkurz closed 2 years ago

scottkurz commented 2 years ago

Signed-off-by: Scott Kurz skurz@us.ibm.com

Fixes #17

scottkurz commented 2 years ago

Thx @rmannibucau for the review. 

1. // e.g. via static field injection` not sure static helps even if I see what you mean, thing is that a) the field is not static (for java language) and you can also get it injected in a constructor, setter or even do a programmatic lookup with the `BeanManager` or `CDI` so `any CDI injection mecanism` sounds more accurate (I know it is a bit picky but CDI is not only fields ;))

Right, I was trying to show field injection as just one example. I fixed the comment so hopefully this is clearer now.

2. I see no words on existing apps with a producer for `JobOperator`, do we want implementations to register their bean only if there is no bean in CDI context matching this (`JobOperator` type + `@Default` qualifier) for backward compat? I think so but can also be a vendor thing, no strong opinion but thought mentioning it can be worth it if missed.

Good point. Let me think about it.

The language is very vague:

Via a CDI Bean, which the batch runtime must provide if necessary.

(and I used similar "must provide if necessary" language for the property/context beans).

So it's probably good enough if we really don't want to say much here.

But what do we want to say? BatchRuntime.getJobOperator() specifies use of ServiceLoader. Is there something we should specify or is it just up to the vendor. Thinking...

rmannibucau commented 2 years ago

Think the impl itself is up to the vendor but the fact the bean is not registered if already found in cdi context can be in the spec

scottkurz commented 2 years ago

After thinking about it some more, I'd prefer to leave this to the vendor and to say nothing about specifying a precedence or ordering here.

I can appreciate the reason to say something like "the batch runtime should NOT register a JobOperator bean if it finds one already registered". It would seem to guarantee that an application that's injecting its own JobOperator currently will continue to be able to do so.

So while that makes sense, I would still rather proceed more slowly.

To start, I'm not sure how to implement this. Does it need to be done during bean discovery, i.e. avoid registering? Or can you register both and then lazily return from one allowing the other producer to get control? I'd have to research. If the former, what does that imply about the ordering upon app initialization? If the latter, what if two impls each beans think they are the ones that should defer to the other?

Probably you could answer those types of objections pretty easily. Still, this is no longer trivial, in my mind, like it was when we were just being vague about this and not saying anything.

Also, this is establishing a pattern for providing spec/API-provided beans with SPI/extension-like considerations (like we're discussing) to consider. If there are no precedents now in the platform, we should talk it through with the CDI and Platform communities to see what they think before creating one.

In waiting until a future release, we also might get some feedback whether this is even important to end users, or whether they are happy to adapt to vendor-specific approaches here.

Anyway, if I'm not convincing you, let's take it to the mailing list and see if we can get any other opinions.

Thanks for bringing it up.

rmannibucau commented 2 years ago

If we dont do it, it means we dont provide a job operator bean right? If so ok for me since it does not break anything.

Technically to impl you just implement a cdi extension which observes processbean or processannotatedtype matching joboperator and if any prevent the afterbeandiscovery registration of the provider bean, nothing crazy, bval impls regularly do it.

scottkurz commented 2 years ago

The discussion since moved to https://www.eclipse.org/lists/jakartabatch-dev/msg00258.html and continued there...

I'm going to merge the present PR and open a follow-up PR to discuss the precedence btw. runtime and app-provided JobOperator Bean(s).

I'm going to release the TCK M1 release reflecting the current round of changes and opened up new issue: https://github.com/eclipse-ee4j/batch-api/issues/190 to follow-up for the final 2.1 release.