From: kedar <Kedar.Mhaswade_at_Sun.COM>
Date: Fri, 09 Oct 2009 11:39:28 -0700

Hi Ken,

Ken Paulsen wrote:
>
> Hi Kedar,
>
> In the console, when we edit these properties, the user would probably
> prefer that we show them the ${variable} and not (only?) the value. So
> I think this will change the behavior of the console in some cases, yes?

I am hoping it won't, because I tested it with GUI and when I changed the
port to ${PORT2} with PORT2 defined to something ("1234") and I see what
attached screen-shots show. Regarding how GUI should really handle system
properties, I have my thoughts (i.e. they should show both resolved and
unresolved values in the same screen rather than making users go elsewhere),
but that's a separate thing.

>
> Review notes to your code below:
>
Thanks!

> 1) Of course, remove the System.out.println's. :)

Yes.

>
> 2) Is the following valid for a single property:
>
> ${host}:${port}
>
> If so, your implementation will not handle that, it only finds a single ${}.

Yeah, you are third person who saw this. I will fix it. I guess I was eager to
see this functionality in action more than making sure it is correct :(

Regards,
Kedar

>
> This is great functionality to have by default! Nice work.
>
> Ken
>
> kedar wrote:
>> Guys,
>>
>> I have made a code change (attached) and tested it and it works
>> just like what we want. I have sent it to Jerome for his review. With
>> this change config listeners will get a chance to see the config
>> beans they get from events as normal config beans. If someone in
>> runtime is till interested in the tokenized value i.e. "${PORT}" and
>> not "1234", then they can query the raw attribute on the bean by
>> getting to Dom inside.
>>
>> e.g.
>> ConfigBeanProxy ls = ...; //(say instance of Network Listener interface)
>> Dom dom = Dom.unwrap(ls); //corresponding "Dom"
>> dom.rawAttribute("port"); //returns "${PORT}".
>>
>> either of:
>> ls.getPort() or dom.attribute("port") will return "1234", which is what
>> the listener is really interested in.
>>
>> I don't think the runtime will ever need unresolved values (i.e.
>> "${PORT"},
>> they are almost always interested in resolved ones).
>>
>> Jerome, please review and let me know if this is something we should do.
>>
>> Regards,
>> Kedar
>>
>> PS - If someone is interested in seeing this in action, please put the
>> attached config.jar in your modules folder and let me know if it works
>> for you.
>>
>> hk2 Code-diff's:
>> Index: config/src/java/org/jvnet/hk2/config/DomDocument.java
>> ===================================================================
>> --- config/src/java/org/jvnet/hk2/config/DomDocument.java (revision
>> 1223)
>> +++ config/src/java/org/jvnet/hk2/config/DomDocument.java (working
>> copy)
>> @@ -59,7 +59,7 @@
>> * configuration is parsed, so this allows circular references
>> &mdash;
>> * {_at_link Translator} may refer to objects in the configuration
>> file being read.
>> */
>> - private volatile Translator translator = Translator.NOOP;
>> + private volatile Translator translator = Translator.SYS_PROP_TR;
>>
>> protected final Map<Inhabitant<? extends
>> ConfigInjector>,ConfigModel> models = new HashMap<Inhabitant<? extends
>> ConfigInjector>, ConfigModel>();
>> private final MultiMap<Class, List<ConfigModel>> implementorsOf =
>> new MultiMap<Class, List<ConfigModel>>();
>> Index: config/src/java/org/jvnet/hk2/config/Translator.java
>> ===================================================================
>> --- config/src/java/org/jvnet/hk2/config/Translator.java (revision
>> 1223)
>> +++ config/src/java/org/jvnet/hk2/config/Translator.java (working
>> copy)
>> @@ -56,4 +56,26 @@
>> return str;
>> }
>> };
>> + public static final Translator SYS_PROP_TR = new Translator() {
>> + public String translate(String str) {
>> + String start = "${";
>> + String end = "}";
>> + int starti = str.indexOf(start);
>> + int endi = str.indexOf(end);
>> + if (starti >= 0 && endi >= 3 ) {
>> + String pn = str.substring(starti+2, endi);
>> + System.out.println("prop name: " + pn);
>> + String pv = System.getProperty(pn);
>> + if (pv == null) {
>> + System.out.println("not replaced: " + str);
>> + return str;
>> + }
>> + //throw new RuntimeException("Define the property
>> first");
>> + String replaced = str.replace(start + pn + end, pv);
>> + System.out.println("replaced: " + replaced);
>> + return replaced;
>> + }
>> + return str; //do nothing otherwise
>> + }
>> + };
>> }
>>
>>
>> Lloyd Chambers wrote:
>>> Shouldn't we supply old and new values?
>>>
>>> Translating values introduces a new subtle issue: the real value
>>> could remain unchanged, but the token could be different eg changing
>>> from ${FOO} to ${BAR} with both FOO and BAR equal to the same value.
>>>
>>> Lloyd
>>>
>>> On Oct 8, 2009, at 9:15 PM, Jerome Dochez wrote:
>>>
>>>> kedar wrote:
>>>>>
>>>>>
>>>>> Jerome Dochez wrote:
>>>>>> This is something that would be pretty easy to change in the
>>>>>> config layer but I am a little scared about possible regressions
>>>>>> by changing this now. I am surprised we did not have that raised
>>>>>> before !
>>>>>>
>>>>>> Kedar, I know everything can be potentially tokenized but
>>>>>> realistically how much do we have tokenized right now (i.e. who
>>>>>> will be potentially impacted by a change a behaviour).
>>>>>
>>>>> I agree. But the problem right now is that listeners do something
>>>>> like:
>>>>> listener.getPort() and expect it to be a number. So, all listeners
>>>>> should be changed to call the service to translate the raw values,
>>>>> right?
>>>>>
>>>>> Besides, there is a correctness issue here. When I have:
>>>>>
>>>>> <system-property name="PORT" value="1234"/>
>>>>> ....
>>>>> <network-listener port="${PORT}"/>,
>>>>>
>>>>> when we restart, listener.getPort() returns 1234 correctly, but in
>>>>> case
>>>>> of dynamic reconfig the same call returns "${PORT}". Is that ok?
>>>>>
>>>> I understand perfectly that it is the problem. I suppose we should
>>>> look at all ConfigListener implementations to ensure that they are
>>>> not already doing translation of raw values because if we fix it, we
>>>> will break those.
>>>>>>
>>>>>> I think the safest of course is to keep the current behaviour and
>>>>>> change it post release if we can. There is a service that be can
>>>>>> be used by config listeners to translate raw values.
>>>>>>
>>>>>> jerome
>>>>>>
>>>>>> Tim Quinn wrote:
>>>>>>> Kedar Mhaswade wrote:
>>>>>>>> Tim Quinn wrote:
>>>>>>>>> If the user changes a config value (I tried using the admin
>>>>>>>>> console) that contains a token (such as
>>>>>>>>> ${com.sun.aas.instanceRoot}) then the event delivered to the
>>>>>>>>> "change" method returns the untranslated old and new values,
>>>>>>>>> still containing the tokens.
>>>>>>>>>
>>>>>>>>> Maybe this has been discussed before and I've forgotten... Is
>>>>>>>>> there something the "change" method can do to translate the
>>>>>>>>> values? It seems that the injected configuration object has
>>>>>>>>> already been updated with the translated new value. Is that
>>>>>>>>> expected and reliable?
>>>>>>>>> Thanks.
>>>>>>>>
>>>>>>>> Tim,
>>>>>>>>
>>>>>>>> Incidentally, I had some conversation with Jerome on this just
>>>>>>>> yesterday
>>>>>>>> and I think it is an issue.
>>>>>>>>
>>>>>>>> IMO, The config layer should already resolve (expand) the
>>>>>>>> variable in the configuration object given to the changed method
>>>>>>>> in ConfigListener
>>>>>>>> so that the runtime code does not have to do anything there.
>>>>>>> That's how it is now...at least that's what I saw.
>>>>>>>>
>>>>>>>> Alternatively, the variable expansion is just doing a
>>>>>>>> replacement from
>>>>>>>> system properties. So in the changed method, if a particular
>>>>>>>> attribute
>>>>>>>> contains {xxx} you could replace it with System.property(xxx). I
>>>>>>>> have
>>>>>>>> tried it and it seems to work.
>>>>>>> I know that the changed method accepts a
>>>>>>> java.beans.PropertyChangeEvent argument. If only it were an
>>>>>>> org.jvnet.hk2.config.PropertyChangeEvent we could have exposed
>>>>>>> getRaw[Old | New]Value methods, for example. Maybe there could
>>>>>>> be a utility method so this could be implemented once instead of
>>>>>>> in each ConfigListener?
>>>>>>> This would be especially helpful if, as Lloyd said, the
>>>>>>> translation should include the <system-property> settings - or
>>>>>>> are those already set so they are accessible via
>>>>>>> System.getProperty()?
>>>>>>>>
>>>>>>>> I am going to spend some time on making the injected configuration
>>>>>>>> object have the variable-expanded values though.
>>>>>>> But it seems to do that already. Am I missing something?
>>>>>>>
>>>>>>> - Tim
>>>>>>
>>>>>>
>>>>>> ---------------------------------------------------------------------
>>>>>> To unsubscribe, e-mail: dev-unsubscribe_at_glassfish.dev.java.net
>>>>>> For additional commands, e-mail: dev-help_at_glassfish.dev.java.net
>>>>>>
>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: dev-unsubscribe_at_glassfish.dev.java.net
>>>>> For additional commands, e-mail: dev-help_at_glassfish.dev.java.net
>>>>>
>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: dev-unsubscribe_at_glassfish.dev.java.net
>>>> For additional commands, e-mail: dev-help_at_glassfish.dev.java.net
>>>>
>>>
>>> Lloyd Chambers
>>> lloyd.chambers_at_sun.com
>>> GlassFish Team
>>>
>>>
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe_at_glassfish.dev.java.net
>>> For additional commands, e-mail: dev-help_at_glassfish.dev.java.net
>>>
>> ------------------------------------------------------------------------
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe_at_glassfish.dev.java.net
>> For additional commands, e-mail: dev-help_at_glassfish.dev.java.net
>>
> --------------------------------------------------------------------- To
> unsubscribe, e-mail: dev-unsubscribe_at_glassfish.dev.java.net For
> additional commands, e-mail: dev-help_at_glassfish.dev.java.net