Vote is cancelled, we also still miss a binding +1

LieGrue,
strub

> Am 08.08.2017 um 11:40 schrieb 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
> b.) the default naming (name="") would break it anyway. You again would need 
> to create dynamic qualifiers for each of those injection points. 
> 
> 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