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

Suggestion: Collect builders and config annotations into main interfaces #41

Closed
aguibert opened this issue Nov 13, 2018 · 3 comments · Fixed by #44
Closed

Suggestion: Collect builders and config annotations into main interfaces #41

aguibert opened this issue Nov 13, 2018 · 3 comments · Fixed by #44
Assignees
Labels
enhancement New feature or request

Comments

@aguibert
Copy link
Contributor

aguibert commented Nov 13, 2018

Currently MP Concurrency has the pattern of 3 classes per object:

  • core interface (e.g. ManagedExecutor)
  • config annotation (e.g. @ManagedExecutorConfig)
  • builder interface (e.g. ManagedExecutorBuilder)

To make it a bit more obvious that these objects go together, I propose that we collect the config annotation and builder interface into the core interface as inner-classes, like this:

// Single import statement for core, config, and builder
import org.eclipse.microprofile.concurrent.ManagedExecutor;

public class MyBean {
    @Inject
    @ManagedExecutor.Config(maxAsync = 5)
    public ManagedExecutor ex;
    
    @Produces
    public ManagedExecutor defaultExec() {
    		return ManagedExecutor.Builder.instance()
    			.maxAsync(5)
    			.build();
    }
}
@njr-11
Copy link
Contributor

njr-11 commented Nov 13, 2018

Thanks for the suggestion. In general this is a good idea, but the CDI qualifier annotation @ManagedExecutorConfig will soon end up with a dependency on CDI (which isn't there currently, but it is being fixed in another pull) and this proposal would result in the MP Concurrency spec having a main path dependency on CDI, which we are trying to avoid.

@njr-11
Copy link
Contributor

njr-11 commented Nov 15, 2018

My comment above no longer applies. We will not be using Qualifier. However @ManagedExecutor.Config looks awkward. Moving the builder under ManageedExecutor does seem reasonable.

@manovotn
Copy link
Contributor

I think moving the build class as an inner class is good idea as it makes it easier to use, +1 for that.
But I would keep the annotation as a separate class.

@njr-11 njr-11 added the enhancement New feature or request label Nov 15, 2018
@njr-11 njr-11 self-assigned this Nov 15, 2018
njr-11 added a commit to njr-11/microprofile-context-propagation that referenced this issue Nov 15, 2018
Signed-off-by: Nathan Rauh <nathan.rauh@us.ibm.com>
njr-11 added a commit to njr-11/microprofile-context-propagation that referenced this issue Nov 16, 2018
Signed-off-by: Nathan Rauh <nathan.rauh@us.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants