2017-08-08 11:40 GMT+02:00 Mark Struberg <strub...@yahoo.de>:

> 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 <rmannibu...@gmail.com
> >:
> >
> > 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 <strub...@yahoo.de>:
> > 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 <strub...@yahoo.de>:
> > >
> > > 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 <johndam...@apache.org>:
> > >>
> > >> 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 <strub...@yahoo.de>
> 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 <johndam...@apache.org>:
> > >>
> > >>>
> > >>> 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 <strub...@yahoo.de>
> 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 <strub...@yahoo.de>:
> > >>>>
> > >>>> And my own +1 of course.
> > >>>>
> > >>>> LieGrue,
> > >>>> strub
> > >>>>
> > >>>>
> > >>>>> Am 01.08.2017 um 11:16 schrieb Reinhard Sandtner <
> reinhard.sandt...@gmail.com>:
> > >>>>>
> > >>>>> +1 (non-binding)
> > >>>>>
> > >>>>> lg
> > >>>>> reini
> > >>>>>
> > >>>>>> Am 30.07.2017 um 12:16 schrieb Jean-Baptiste Onofré <
> j...@nanthrax.net>:
> > >>>>>>
> > >>>>>> +1 (non binding)
> > >>>>>>
> > >>>>>> Regards
> > >>>>>> JB
> > >>>>>> On Jul 30, 2017, at 12:04, Jean-Louis MONTEIRO <
> jeano...@gmail.com> wrote:
> > >>>>>> +1
> > >>>>>> Thanks
> > >>>>>>
> > >>>>>> Le ven. 28 juil. 2017 à 13:20, Romain Manni-Bucau <
> rmannibu...@gmail.com> 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 <strub...@yahoo.de>:
> > >>>>>> Yes we should make sure this gets propagated to dist!
> > >>>>>>
> > >>>>>> LieGrue,
> > >>>>>> strub
> > >>>>>>
> > >>>>>>> Am 28.07.2017 um 13:58 schrieb John D. Ament <
> johndam...@apache.org>:
> > >>>>>>>
> > >>>>>>> +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 <strub...@yahoo.de>
> 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
> > >>>>>>>
> > >>>>>>
> > >>>>>>
> > >>>>>
> > >>>>
> > >>>
> > >
> >
> >
>
>

Reply via email to