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