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

CDI-650 Introduce asynchronous event notification options #337

Merged
merged 1 commit into from Nov 29, 2016

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Nov 22, 2016

@mkouba
Copy link
Contributor Author

mkouba commented Nov 23, 2016

@cdi-spec/eg please review...

@manovotn
Copy link
Contributor

After quite a lengthy conversation we had on this topic yesterday with Martin and Tomas, I give this idea +1.
The reason is that:

  • In the future we do want to add more stuff/options to events such as timeouts.
  • However we cannot push such things into spec now, as it is far too late for such changes and we are unsure about implementation aspects (it might get really complex).
  • This solution keeps the door open for CDI implementations. It covers current options such as custom executors and allows for unlimited other options which the given impl chooses to add. So even with spec being released, we might keep adding stuff at impl level which we might later (2.1) want to standardize.
  • Last but not least, standardizing the above^ is very easy as all that is needed is to add a method to dedicated interface.

Copy link
Contributor

@antoinesd antoinesd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 as well. We'll probably be asked to make CDI more "reactive" in the next release(s), providing a way to add features in that direction without bloating the spec is a good approach.
IMO We need a specific doc for the NotificationOptions in the spec and an perhaps a way to access it on the observer side (thru EventMetadata?)

@antoinesd antoinesd dismissed their stale review November 25, 2016 09:57

Closed too soon

@mkouba
Copy link
Contributor Author

mkouba commented Nov 25, 2016

@antoinesd

We need a specific doc for the NotificationOptions in the spec

I find the current wording (attached to fireAsync()) sufficient. There's not much we can tell about NotificationOptions right now.

and an perhaps a way to access it on the observer side (thru EventMetadata?)

I'm not so sure and I would prefer keep things simple in 2.0 - we can extend EventContext in 2.1 if needed.

@antoinesd
Copy link
Contributor

antoinesd commented Nov 25, 2016

Ok, let's keep more advanced feature for 2.1.

Copy link
Contributor

@antoinesd antoinesd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Emily-Jiang
Copy link
Contributor

In the javadoc, "The container is permitted to define other non-portable notification options.", will this encourage containers to define more and more non-portable behaviours? I think we should leave it out, as I suggest we spec the options in CDI 2.1.

@mkouba
Copy link
Contributor Author

mkouba commented Nov 25, 2016

will this encourage containers to define more and more non-portable behaviours?

We must leave some space for experiments so that impls can test options that can be later standardized.

@Emily-Jiang
Copy link
Contributor

I understand that. Do we need to doc this 'freedom'?

@mkouba
Copy link
Contributor Author

mkouba commented Nov 25, 2016

I think so, BTW we already use similar wording in the spec -> CTRL + F the container is permitted.

@Emily-Jiang
Copy link
Contributor

ok. No further comments. LGTM

@antoinesd antoinesd merged commit 7a0c932 into jakartaee:master Nov 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants