Tim,
Tim Quinn wrote:
> Kedar,
>
> Just to be very specific, by "just like what we want" do you mean that
> both the oldValue and the newValue from the config change event will be
> translated?
No, by this change, the "beans" inside the events will be made analogous
to the ones that you get via normal injection. The oldValue and newValue
in event is not changed because of this change I am proposing. So, suppose
the value of "port" attribute is "8080" and you changed it to "${PORT}, those
will be the oldValue and newValue respectively. Doing bean.getPort() however
will return "1234" if a system-property is defined with PORT=1234.
>
> The translate method you propose handles only one placeholder in the
> value. I'm not sure of an example, but couldn't there be cases in which
> a string contained multiple placeholders to be replaced? Or do you
> expect the translate method to be invoked multiple times for each value
> until the return value is unchanged from the argument?
Good catch! Bill saw the same thing. The translate method is called
only once when you call a particular attribute on a bean. I will fix that.
But can you please give a try to config.jar and let me know that in your
config listener you are seeing the thing you expect?
Regards,
Kedar
>
> - Tim
>
> 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
>> —
>> * {_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
>