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?
Review notes to your code below:
1) Of course, remove the System.out.println's. :)
2) Is the following valid for a single property:
${host}:${port}
If so, your implementation will not handle that, it only finds a single
${}.
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
—
* {@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@glassfish.dev.java.net
For additional commands, e-mail: dev-help@glassfish.dev.java.net
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@glassfish.dev.java.net
For additional commands, e-mail: dev-help@glassfish.dev.java.net
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@glassfish.dev.java.net
For additional commands, e-mail: dev-help@glassfish.dev.java.net
Lloyd Chambers
lloyd.chambers@sun.com
GlassFish Team
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@glassfish.dev.java.net
For additional commands, e-mail: dev-help@glassfish.dev.java.net
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@glassfish.dev.java.net
For additional commands, e-mail: dev-help@glassfish.dev.java.net