Kedar,
The issue is that AMX *must* deal with the RAW attributes, and the GUI
expects this. So getters and setters always deal in raw values, but
AMXConfig MBeans also implement an AttributeResolver interface that
resolves ${...}.
Now when it comes to Notifications, AMX needs to be consistent with
that: provide old and new values that are ${...}, not the resolved
values.
But one of your previous emails indicates this might not be a problem
in that the values carried along are actually the ${...} stuff.
This is what AMX does to emit a notification. If event.getOld/NewValue
() yields RAW values, then all is well.
for ( final PropertyChangeEvent event : remainingEvents)
{
final Object oldValue = event.getOldValue();
final Object newValue = event.getNewValue();
final Object source = event.getSource();
final String propertyName = event.getPropertyName();
//final String sourceString = (source instanceof
ConfigBeanProxy) ? ConfigSupport.proxyType((ConfigBeanProxy)
source).getName() : "" + source;
//debug( "AMXConfigLoader.sortAndDispatch (ATTR change): name
= " + propertyName +
// ", oldValue = " + oldValue + ", newValue = " +
newValue + ", source = " + sourceString );
if ( source instanceof ConfigBeanProxy )
{
// CHANGE
final ConfigBeanProxy cbp = (ConfigBeanProxy)source;
final ConfigBean cb = asConfigBean( ConfigBean.unwrap
( cbp ) );
//final Class<? extends ConfigBeanProxy> proxyClass =
ConfigSupport.proxyType(cbp);
// change events without prior add
// we shouldn't have to check for this, but it's a bug in
the caller: no even for
// new ConfigBean, but changes come along anyway
if ( mRegistry.getObjectName(cb) == null )
{
if ( ! newConfigBeans.contains(cb) )
{
//debug( "AMXConfigLoader.sortAndDispatch:
process new ConfigBean (WORKAROUND): " + proxyClass.getNameProp() );
handleConfigBean( cb, false );
newConfigBeans.add( cb );
}
}
else
{
issueAttributeChange( cb, propertyName, oldValue,
newValue, whenChanged);
}
}
else
{
debug( "AMXConfigLoader.sortAndDispatch: WARNING: source
is not a ConfigBean" );
}
}
private void
issueAttributeChange(
final ConfigBean cb,
final String xmlAttrName,
final Object oldValue,
final Object newValue,
final long whenChanged )
{
final ObjectName objectName = mRegistry.getObjectName(cb);
if ( objectName == null )
{
throw new IllegalArgumentException( "Can't issue attribute
change for null ObjectName for ConfigBean " + cb.getProxyType().getName
() );
}
boolean changed = false;
if ( oldValue != null )
{
changed = ! oldValue.equals(newValue);
}
else if ( newValue != null )
{
changed = ! newValue.equals(oldValue);
}
if ( changed )
{
//debug( "AMXConfigLoader.issueAttributeChange(): " +
xmlAttrName + ": {" + oldValue + " => " + newValue + "}");
final Object impl = mRegistry.getImpl(cb);
if ( ! (impl instanceof AMXConfigImpl) )
{
throw new IllegalStateException("impossible");
}
final AMXConfigImpl amx = (AMXConfigImpl)impl;
final String message = cb.getProxyType().getName() + "." +
xmlAttrName + ": " + oldValue + " => " + newValue;
amx.issueAttributeChangeForXmlAttrName( xmlAttrName, message,
oldValue, newValue, whenChanged );
}
}
Lloyd
On Oct 9, 2009, at 11:20 AM, kedar wrote:
> 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
>
> ---------------------------------------------------------------------
> 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