You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomee.apache.org by David Blevins <da...@visi.com> on 2006/10/24 22:14:49 UTC
SystemInstance, main args & random fun (was: Re: svn commit: r467149 - in /incubator/openejb/trunk/openejb3: container/openejb-core/ container/openejb-core/src/main/java/org/apache/openejb/ container/openejb-core/src/main/java/org/apache/openejb/alt/config/ container/openejb-core/src/main/java/org/ap...)
Nice changes. You're a coding maniac all of the sudden! You feeling
ok? ;)
Some random thoughts/ideas below.
> URL: http://svn.apache.org/viewvc/incubator/openejb/trunk/openejb3/
> container/openejb-core/src/main/java/org/apache/openejb/
> OpenEJB.java?view=diff&rev=467149&r1=467148&r2=467149
> ======================================================================
> ========
> --- incubator/openejb/trunk/openejb3/container/openejb-core/src/
> main/java/org/apache/openejb/OpenEJB.java (original)
> +++ incubator/openejb/trunk/openejb3/container/openejb-core/src/
> main/java/org/apache/openejb/OpenEJB.java Mon Oct 23 15:10:17 2006
> @@ -83,7 +83,7 @@
> } catch (java.io.IOException e) {
> }
> if (initProps.getProperty("openejb.nobanner") == null) {
> - System.out.println("OpenEJB " + versionInfo.get
> ("version") + " build: " + versionInfo.get("date") + "-" +
> versionInfo.get("time"));
> + System.out.println("Apache OpenEJB " +
> versionInfo.get("version") + " build: " + versionInfo.get
> ("date") + "-" + versionInfo.get("time"));
For the longest time i've wanted to encapsulate that version info
properties object with a strongly typed object. i.e.
versionInfo.getDate(), versionInfo.getTime(), versionInfo.getVersion
(), and so on. Since we moved to SVN as well, I've always really
really wanted to have the svn revision printed out as well. If you
want to work on any of that, that'd be awesome.
> URL: http://svn.apache.org/viewvc/incubator/openejb/trunk/openejb3/
> container/openejb-core/src/main/java/org/apache/openejb/alt/config/
> Deploy.java?view=diff&rev=467149&r1=467148&r2=467149
> ======================================================================
> ========
> --- incubator/openejb/trunk/openejb3/container/openejb-core/src/
> main/java/org/apache/openejb/alt/config/Deploy.java (original)
> +++ incubator/openejb/trunk/openejb3/container/openejb-core/src/
> main/java/org/apache/openejb/alt/config/Deploy.java Mon Oct 23
> 15:10:17 2006
> @@ -27,7 +27,6 @@
[...]
> } else if (args[i].equals("-l")) {
> if (args.length > i + 1) {
> - System.setProperty("log4j.configuration",
> args[++i]);
> + system.setProperty("log4j.configuration",
> args[++i]);
> }
> } else if .....
Note that if someone actually uses the -l option it won't work as
log4j will be looking in the system properties. This is part of the
tricky bits that I was mentioning the other day. I'm totally on the
fence as what to do with this one, maybe you have some ideas.
The basic options for code that needs to set actual system properties
are either 1) have code that use System directly or 2) have
SystemInstance "know" when to do that for them and do it for them.
The downside of #1 is it's perhaps a bit messy having
System.getProperty and SystemInstance.getProperty usage right next to
each other. Seems it'd be awfully easy to make a mistake and use the
wrong one.
You were leaning towards #2 in your PropertiesService class where it
would delegate to the System properties if not found in the
SystemInstance, and I generally liked the idea. The parts that makes
me wonder if it's too complicated are:
Get/set symmetry:
I.e get and set should be identical. So set would have to be
coded to know in advance what properties to pass through to the
System properties and which to keep for itself.
si.getProperties().[gs]etProperty() and si.[gs]etProperty() symmetry:
I.e. these two should behave exactly the same..
systemInstance.getProperty("log4j.configuration")
systemInstance.getProperties().getProperty("log4j.configuration")
As should these..
systemInstance.setProperty("log4j.configuration", foo)
systemInstance.getProperties().setProperty("log4j.configuration",
foo)
I think I still like the #2 better but it'd be more work. What do
you think?
> URL: http://svn.apache.org/viewvc/incubator/openejb/trunk/openejb3/
> container/openejb-core/src/main/java/org/apache/openejb/cli/
> Main.java?view=diff&rev=467149&r1=467148&r2=467149
> ======================================================================
> ========
> --- incubator/openejb/trunk/openejb3/container/openejb-core/src/
> main/java/org/apache/openejb/cli/Main.java (original)
> +++ incubator/openejb/trunk/openejb3/container/openejb-core/src/
> main/java/org/apache/openejb/cli/Main.java Mon Oct 23 15:10:17 2006
> @@ -17,6 +17,7 @@
>
[...]
> + * TODO: There must be a better way to read command line args
> and spawn a command
There's always a better way to do anything. What did you have in mind?
> + */
> public static void main(String[] args) {
> - ArrayList argsList = new ArrayList();
[...]
> + // FIXME: Why do we bother to process env vars? Remove it
> and leave it to JVM
Just a nice trick so people can specify -D stuff anywhere on the
command line. Stole the idea from Maven. I.e. these use the same
technique:
openejb start -Dopenejb.localcopy=false
maven clean build -Dmaven.test.skip=true
> URL: http://svn.apache.org/viewvc/incubator/openejb/trunk/openejb3/
> container/openejb-loader/src/main/java/org/apache/openejb/loader/
> SystemInstance.java?view=diff&rev=467149&r1=467148&r2=467149
> ======================================================================
> ========
> --- incubator/openejb/trunk/openejb3/container/openejb-loader/src/
> main/java/org/apache/openejb/loader/SystemInstance.java (original)
> +++ incubator/openejb/trunk/openejb3/container/openejb-loader/src/
> main/java/org/apache/openejb/loader/SystemInstance.java Mon Oct 23
> 15:10:17 2006
> @@ -38,11 +38,12 @@
> private final FileUtils home;
> private final FileUtils base;
> private final ClassLoader classLoader;
> - private final HashMap components;
> + private final HashMap<Class, Object> components;
> private final ClassPath classPath;
> -
> +
> + // FIXME: Why is Exception thrown at all? It's almost
> impossible that it'll happen.
I don't recall why I did that. I must have written then yanked some
code in the constructor that threw an exception and just left the
throws claus in. I do that sometimes. Feel free to yank it too.
> private SystemInstance(Properties properties) throws Exception {
> - this.components = new HashMap();
> + this.components = new HashMap<Class, Object>();
> this.properties = new Properties();
> this.properties.putAll(System.getProperties());
> this.properties.putAll(properties);
> @@ -111,7 +112,6 @@
> }
> + public boolean isPropertySet(String propName) {
> + return this.properties.get(propName) != null;
> }
I like this. What do you think about renaming it to 'hasProperty'?
I've been disliking the SystemInstance.init method more and more.
I'd really like to kill it but i seem to recall some part of the
tomcat integration really needed it, but I really don't remember the
details. If that's something you want to experiment with, that'd be
cool. Just make sure you build the assemblies run the "maven itest"
on the tomcat ones if you do.
That's all from me for now, just random thoughts.
-David