Lloyd,
I am all for deferring this if it is causing too much of pain. My hope
is that it won't, but it needs more analysis, I agree. In particular,
I need to understand better what AMX is doing in this regard.
Also, we have CLI path, which currently does not leverage AMX. When I
tested this with CLI, e.g.
1- create a system-property using asadmin create-system-properties ("PORT=1234")
2- restart (I need to make this dynamic, I will do it shortly).
3- asadmin set "....http-listener-1.port=${PORT}"
I tried the same and this time, instead of 3) above, I set the attribute port on
http-listener-1 on the admin GUI screen.
Before this code change, you'd see a NumberFormatException, with this change,
http-listener-1 binds to 1234 correctly *dynamically*.
I thought testing it via both CLI and GUI is enough to declare the victory :)
Anyway, let me take it offline with you, Tim, Jerome and whoever is interested.
I am not pushing for this change, but if you can try it out with config.jar I
sent around that will *really* help!
Regards,
Kedar
Lloyd Chambers wrote:
> 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
>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe_at_glassfish.dev.java.net
> For additional commands, e-mail: dev-help_at_glassfish.dev.java.net
>