Yair Zaslavsky has posted comments on this change.
Change subject: engine: Get Gluster Servers query
......................................................................
Patch Set 16: (9 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GetGlusterServersQuery.java
Line 52:
Line 53: try {
Line 54: client = connect(getParameters().getServerName());
Line 55: validateFingerprint(client,
getParameters().getFingerprint());
Line 56: authenticate(client, "root",
getParameters().getRootPassword());
Any reason why not to add this as const as well?
Line 57: String serversXml = executeCommand(client);
Line 58:
getQueryReturnValue().setReturnValue(extractServers(serversXml));
Line 59: } catch (Exception e) {
Line 60: throw new RuntimeException(e);
Line 73: throw new RuntimeException("Could not get the peer list
form the host: " + getParameters().getServerName());
Line 74: }
Line 75: }
Line 76:
Line 77: private Document loadXmlDoc(String serversXml) throws
ParserConfigurationException, SAXException, IOException {
IMHO, should be extracted to XmlUtils at utils module.
Line 78: DocumentBuilderFactory docBuilderFactory =
DocumentBuilderFactory.newInstance();
Line 79: DocumentBuilder docBuilder =
docBuilderFactory.newDocumentBuilder();
Line 80: InputSource is = new InputSource(new StringReader(serversXml));
Line 81: Document doc = docBuilder.parse(is);
Line 127: client.setHost(serverName, PORT);
Line 128: try {
Line 129: client.connect();
Line 130: return client;
Line 131: } catch (Exception e) {
Don't you want to add some logging here? at least at debug level?
Line 132: throw new RuntimeException(e);
Line 133: }
Line 134: }
Line 135:
Line 167:
Line 168: return OpenSSHUtils.getKeyFingerprintString(serverKey);
Line 169: }
Line 170:
Line 171: private int getIntValue(Element element, String tagName) {
Please extract to XmlUtils
Line 172: return Integer.parseInt(getTextValue(element, tagName));
Line 173: }
Line 174:
Line 175: private String getTextValue(Element element, String tagName) {
Line 171: private int getIntValue(Element element, String tagName) {
Line 172: return Integer.parseInt(getTextValue(element, tagName));
Line 173: }
Line 174:
Line 175: private String getTextValue(Element element, String tagName) {
Please extract to XmlUtils
Line 176: return
element.getElementsByTagName(tagName).item(0).getChildNodes().item(0).getNodeValue().trim();
Line 177: }
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/gluster/GetGlusterServersQueryTest.java
Line 27: SSHClient clientMock;
Line 28: String serverName2 = "testserver2";
Line 29: String fingerprint1 =
"31:e2:1b:7e:89:86:99:c3:f7:1e:57:35:fe:9b:5c:31";
Line 30: String fingerprint2 =
"31:e2:1b:7e:89:86:99:c3:f7:1e:57:35:fe:9b:5c:32";
Line 31: private static int CONNECT_TO_SERVER_TIMEOUT = 20;
any reason why this is not final?
Line 32: private static String GLUSTER_PEER_STATUS_CMD = "gluster peer
status --xml";
Line 33: String outputXml =
Line 34:
"<cliOutput><peerStatus><peer><uuid>85c42b0d-c2b7-424a-ae72-5174c25da40b</uuid><hostname>testserver1</hostname><connected>1</connected><state>3</state></peer>"
Line 35: +
Line 28: String serverName2 = "testserver2";
Line 29: String fingerprint1 =
"31:e2:1b:7e:89:86:99:c3:f7:1e:57:35:fe:9b:5c:31";
Line 30: String fingerprint2 =
"31:e2:1b:7e:89:86:99:c3:f7:1e:57:35:fe:9b:5c:32";
Line 31: private static int CONNECT_TO_SERVER_TIMEOUT = 20;
Line 32: private static String GLUSTER_PEER_STATUS_CMD = "gluster peer
status --xml";
any reason why this is not final?
Line 33: String outputXml =
Line 34:
"<cliOutput><peerStatus><peer><uuid>85c42b0d-c2b7-424a-ae72-5174c25da40b</uuid><hostname>testserver1</hostname><connected>1</connected><state>3</state></peer>"
Line 35: +
Line 36:
"<peer><uuid>85c42b0d-c2b7-424a-ae72-5174c25da40b</uuid><hostname>testserver2</hostname><connected>1</connected><state>3</state></peer></peerStatus></cliOutput>";
Line 29: String fingerprint1 =
"31:e2:1b:7e:89:86:99:c3:f7:1e:57:35:fe:9b:5c:31";
Line 30: String fingerprint2 =
"31:e2:1b:7e:89:86:99:c3:f7:1e:57:35:fe:9b:5c:32";
Line 31: private static int CONNECT_TO_SERVER_TIMEOUT = 20;
Line 32: private static String GLUSTER_PEER_STATUS_CMD = "gluster peer
status --xml";
Line 33: String outputXml =
Any reason why not to define it as static final?
Line 34:
"<cliOutput><peerStatus><peer><uuid>85c42b0d-c2b7-424a-ae72-5174c25da40b</uuid><hostname>testserver1</hostname><connected>1</connected><state>3</state></peer>"
Line 35: +
Line 36:
"<peer><uuid>85c42b0d-c2b7-424a-ae72-5174c25da40b</uuid><hostname>testserver2</hostname><connected>1</connected><state>3</state></peer></peerStatus></cliOutput>";
Line 37:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/gluster/GlusterServersQueryParameters.java
Line 31: public void setServerName(String serverName) {
Line 32: this.serverName = serverName;
Line 33: }
Line 34:
Line 35: public String getRootPassword() {
Why not have a field named password with appropriate getter and setter? why if
in the future someone would like to work with a user that is not root?
Line 36: return rootPassword;
Line 37: }
Line 38:
Line 39: public void setRootPassword(String rootPassword) {
--
To view, visit http://gerrit.ovirt.org/7584
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic69a9a48bf227c8fa805c8aa9c4f08fa36ea9425
Gerrit-PatchSet: 16
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Dhandapani Gopal <dgo...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Dhandapani Gopal <dgo...@redhat.com>
Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com>
Gerrit-Reviewer: Michael Kublin <mkub...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Selvasundaram <sesub...@redhat.com>
Gerrit-Reviewer: Shireesh Anjal <san...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
