Juan Hernandez has posted comments on this change.
Change subject: core: introduce osinfo service instead of Config values
......................................................................
Patch Set 1: (3 inline comments)
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/osinfo/LibosinfoClient.java
Line 68:
Line 69: libosinfoRMIServer = builder.start();
Line 70:
Line 71: } catch (Exception exception) {
Line 72: System.out.println("Can't start exernal VM.");
I am sure you intended to use log, not System.out.
Line 73: }
Line 74: }
Line 75:
Line 76: private ProcessBuilder buildExternalJavaProcessCmd() {
Line 86:
Line 87: String options = "-Djava.rmi.server.hostname=localhost";
Line 88: String mainClassName =
"org.ovirt.engine.core.osinfo.server.LibosinfoServer";
Line 89:
Line 90: builder.command(java, "-cp", classpath, options,
mainClassName);
Instead of building the classpath in this way, which is error prone, I would
suggest to create a JBoss module for the external server, say that you name it
"org.ovirt.engine.core.osinfo.server, then you can launch it with the following
command line:
builder.command(
java,
"-jar", LocalConfig.getProperty("JBOSS_HOME") + File.separator +
"jboss-modules.jar",
"-mp", System.getProperty("module.path"),
"org.ovirt.engine.core.osinfo.server"
);
This way you don't have to try to locate the "tools.jar" and "utils.jar" files,
as they may move (in fact they have moved, and are no longer under the ear/lib
directory).
Line 91: return builder;
Line 92: }
Line 93:
Line 94: private ProcessBuilder buildShellProcessCmd() {
Line 90: builder.command(java, "-cp", classpath, options,
mainClassName);
Line 91: return builder;
Line 92: }
Line 93:
Line 94: private ProcessBuilder buildShellProcessCmd() {
I assume that this is a method that you use to start the libosinfo server
standalone, for testing purposes, can you add some comments explaining it? That
or remove it from the code. And if you decide to keep this method then the sell
script should also be part of this change.
Line 95: ProcessBuilder builder = new ProcessBuilder();
Line 96: String cmd = LocalConfig.getInstance().getUsrDir().getPath() +
"/bin/engine-libosinfo-server.sh/";
Line 97: return builder.command(cmd);
Line 98: }
--
To view, visit http://gerrit.ovirt.org/12936
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd75679a1a1af5d5a0925e181b5dfd6e87574a75
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Roy Golan <rgo...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
