2017-08-08 11:40 GMT+02:00 Mark Struberg <[email protected]>: > I think the API is quite fine. What are your concrete concerns? > > There are btw 2 reasons why the name in the @ConfigProperty is @Nonbinding: > > a.) you would need to register about 2000 different Beans otherwise (one > per injection point basically >
yep but without much memory impact once factorized properly > b.) the default naming (name="") would break it anyway. You again would > need to create dynamic qualifiers for each of those injection points. > Not true since you handle it in the extension precomputing it My main concern is the API is rigid and under the CDI constraint instead of proposing something: - handled internally by design (fast runtime path) - enforcing user to modelize the config instead of encouraging to spread it - including duplication - accross the whole app - centralizing eviction through the impl instead of having to gather all usages and evict it in best effort which is not giving any guarantee of reliability of the code > > LieGrue, > strub > > > Am 08.08.2017 um 11:34 schrieb Romain Manni-Bucau <[email protected] > >: > > > > we can rollback it, anyway I was not that involved in MP but any hope > the spec change enough to make the runtime pipeline straight forward and > avoid any resolution at that time? Don't think the API is strong ATM > > > > > > Romain Manni-Bucau > > @rmannibucau | Blog | Old Blog | Github | LinkedIn | JavaEE Factory > > > > 2017-08-08 11:33 GMT+02:00 Mark Struberg <[email protected]>: > > Had a talk with Romain via IRC. His goal was to improve the performance > during injection by providing separate Beans for each Injection Point. > > We debugged through and in practice the benefit only pays off if you > have just a single @ConfigProperty Provider<T> for each type T. This will > imo merely increase the performance for very simple apps and samples. So > while the goal is good it' imo not worth it as it will not affect real > world projects. In those you will almost always have multiple > Provider<String> for example. > > > > It was worth a try, but imo the increased complexity (from 50 LOC to 400 > LOC) isn't worth the benefits which only will affect very simple apps > anyway. > > > > Should we go on and ship this version anyway and rework later, or just > do it and qickly re-roll (will not take long)? > > > > LieGrue, > > strub > > > > > Am 08.08.2017 um 09:24 schrieb Mark Struberg <[email protected]>: > > > > > > Yes that's something else I saw yesterday as well. > > > Gonna fix that and reroll. > > > > > > LieGrue, > > > strub > > > > > >> Am 08.08.2017 um 01:24 schrieb John D. Ament <[email protected]>: > > >> > > >> Agreed that it would make sense that this required check is skipped > for provider, optional. Only Optional is called out though. > > >> > > >> At the same time, I think we're over eagerly saying that fields are > not set. I raised a spec issue yesterday, basically if the value is > "org.eclipse.microprofile.config.configproperty.unconfigureddvalue" that > seems to be what indicates its not set. However, GConfig is also blowing > up when it's set to "". > > >> > > >> I don't see an issue fixing this on master. However, right now I > have master targetting config 1.1. We can revert back to 1.0 to cut a 1.1 > shortly. > > >> > > >> John > > >> > > >> On Mon, Aug 7, 2017 at 7:16 PM Mark Struberg <[email protected]> > wrote: > > >> It's a requirement for direct injection, but not for Provider<T> afair > > >> > > >> Lg, > > >> Strub > > >> > > >> Am 08.08.2017 um 01:03 schrieb John D. Ament <[email protected]>: > > >> > > >>> > > >>> I took a look at the test and the commit, to remember why this would > fail. > > >>> > > >>> The test fails because the property isn't set. This is actually a > MP Config requirement, validating injection points. If I add > System.setProperty(SOME_KEY, "no-op"); to the beginning of the deployment > method, it works as expected. See [1] for the API requirements > > >>> > > >>> Now that test continues to fail on Weld3 with this change in, > seemingly the custom provider doesn't work. Are you using Weld or OWB as > your runtime? > > >>> > > >>> [1]: https://github.com/eclipse/microprofile-config/blob/1.0/ > api/src/main/java/org/eclipse/microprofile/config/inject/ > ConfigProperty.java#L50 > > >>> > > >>> > > >>> On Mon, Aug 7, 2017 at 5:27 PM Mark Struberg <[email protected]> > wrote: > > >>> I have now tried to use this version in a project and totally blew > up. > > >>> I reviewed the code in depth and I'm not sure how to fix it except > with a revert of some committs. > > >>> > > >>> So I gonna change my vote to -1 > > >>> > > >>> The reason seems to be r1800744 which imo introduced uneccessary > complexity and broke quite a few features. > > >>> > > >>> e.g. injection of Provider is now broken. > > >>> It also breaks programmatic lookup, etc. > > >>> > > >>> How to proceed? > > >>> > > >>> LieGrue, > > >>> strub > > >>> > > >>> > > >>>> Am 01.08.2017 um 21:54 schrieb Mark Struberg <[email protected]>: > > >>>> > > >>>> And my own +1 of course. > > >>>> > > >>>> LieGrue, > > >>>> strub > > >>>> > > >>>> > > >>>>> Am 01.08.2017 um 11:16 schrieb Reinhard Sandtner < > [email protected]>: > > >>>>> > > >>>>> +1 (non-binding) > > >>>>> > > >>>>> lg > > >>>>> reini > > >>>>> > > >>>>>> Am 30.07.2017 um 12:16 schrieb Jean-Baptiste Onofré < > [email protected]>: > > >>>>>> > > >>>>>> +1 (non binding) > > >>>>>> > > >>>>>> Regards > > >>>>>> JB > > >>>>>> On Jul 30, 2017, at 12:04, Jean-Louis MONTEIRO < > [email protected]> wrote: > > >>>>>> +1 > > >>>>>> Thanks > > >>>>>> > > >>>>>> Le ven. 28 juil. 2017 à 13:20, Romain Manni-Bucau < > [email protected]> a écrit : > > >>>>>> +1 (for the release and the src zip in dist) > > >>>>>> > > >>>>>> > > >>>>>> Romain Manni-Bucau > > >>>>>> @rmannibucau | Blog | Old Blog | Github | LinkedIn | JavaEE > Factory > > >>>>>> > > >>>>>> 2017-07-28 14:17 GMT+02:00 Mark Struberg <[email protected]>: > > >>>>>> Yes we should make sure this gets propagated to dist! > > >>>>>> > > >>>>>> LieGrue, > > >>>>>> strub > > >>>>>> > > >>>>>>> Am 28.07.2017 um 13:58 schrieb John D. Ament < > [email protected]>: > > >>>>>>> > > >>>>>>> +1 to ship it. > > >>>>>>> > > >>>>>>> One comment, there's been input in the past that geronimo > releases don't show up in the system. Should we ensure that the final > result gets put into https://dist.apache.org/repos/dist/release/geronimo/ > ? > > >>>>>>> > > >>>>>>> On Fri, Jul 28, 2017 at 7:25 AM Mark Struberg <[email protected]> > wrote: > > >>>>>>> Hi! > > >>>>>>> > > >>>>>>> Apache geronimo-config is an implementation of the > Microprofile-1.0 Config specification [1][2]. > > >>>>>>> > > >>>>>>> It allows flexible and extensible Configuration for applications. > > >>>>>>> > > >>>>>>> > > >>>>>>> Here is our staging repo > > >>>>>>> https://repository.apache.org/content/repositories/ > orgapachegeronimo-1035/ > > >>>>>>> > > >>>>>>> The Source distribution can be found here: > > >>>>>>> https://repository.apache.org/content/repositories/ > orgapachegeronimo-1035/org/apache/geronimo/config/config-parent/1.0/ > > >>>>>>> > > >>>>>>> > > >>>>>>> Our own tag in SVN is > > >>>>>>> https://svn.apache.org/repos/asf/geronimo/components/ > config/tags/config-parent-1.0/ > > >>>>>>> > > >>>>>>> > > >>>>>>> Please VOTE > > >>>>>>> [+1] yeah, ship it! > > >>>>>>> [+0] meh, don't care > > >>>>>>> [-1] nope, because ${showstopper} > > >>>>>>> > > >>>>>>> The VOTE is open for 72h > > >>>>>>> > > >>>>>>> txs and LieGrue, > > >>>>>>> strub > > >>>>>>> > > >>>>>>> > > >>>>>>> > > >>>>>>> [1] https://github.com/eclipse/microprofile-config > > >>>>>>> [2] https://github.com/eclipse/microprofile-config/releases > > >>>>>>> > > >>>>>> > > >>>>>> > > >>>>> > > >>>> > > >>> > > > > > > > > >
