Now I'm a little concerned.
AMX first looks for a system property via System.getProperty().
If not found, then AMX implements
org.jvnet.hk2.config.VariableResolver (HK2 interface) by looking
through configuration of <system-property>, thus finding the CURRENT
values.
How does using System.getProperty() work at all iof a value changes?
Do we update system properties on the fly?
Is AMX doing this wrong?
I think this change should be deferred until we understand this issue,
as well as the side-effect I mentioned previously.
Lloyd
On Oct 9, 2009, at 10:29 AM, 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
> <
> config.jar
> >---------------------------------------------------------------------
> 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