Alon Bar-Lev has posted comments on this change.
Change subject: engine: Get Gluster Servers query
......................................................................
Patch Set 10: (5 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GetGlusterServersQuery.java
Line 77: String errMsg = "Could not get the peer list form the
host: " + getParameters().getServerName();
Line 78: log.error(errMsg);
Line 79: throw new RuntimeException(errMsg);
Line 80: }
Line 81: } catch (ParserConfigurationException e1) {
Why do you use e1, e2... and not simply e?
Line 82: log.errorFormat("There is problem in parsing xml." +
e1.getMessage());
Line 83: throw new RuntimeException(e1);
Line 84: } catch (SAXException e2) {
Line 85: log.errorFormat("Error in parsing xml." + e2.getMessage());
Line 93: }
Line 94: }
Line 95: }
Line 96:
Line 97: private void validateFingerprint(SSHClient client) {
Yes... it is used only once... and is short enough to be a block at
executeQueryCommand().
Line 98: if
(!getParameters().getFingerprint().equals(getFingerprint(client))) {
Line 99: String errMsg =
Line 100: "SSH Fingerprint of server " +
getParameters().getServerName()
Line 101: + " did not match expected fingerprint "
+ getParameters().getFingerprint();
Line 103: throw new RuntimeException(errMsg);
Line 104: }
Line 105: }
Line 106:
Line 107: protected SSHClient createSSHClient() {
So add a comment... "This is required for mock during test".
Line 108: return new SSHClient();
Line 109: }
Line 110:
Line 111: protected SSHClient connect(String serverName) {
Line 131: client.setUser("root");
Line 132: client.setPassword(getParameters().getRootPassword());
Line 133:
Line 134: try {
Line 135: client.authenticate();
I prefer split this to authenticate and execute, notice that the bellow
exception may be misleading if these are grouped.
The code should be read as a story:
connect
validate fingerprint
authenticate
execute command
These above step should be at same level within one function, by that order to
simplify review and maintenance. This is how the story is to be told... we are
connecting, validating the fingerprint, authenticating, then executing commands.
It is the same story that should be told over and over, whenever ssh is used,
either if one command is executed or 10 commands are executed.
Same story = easier to remember, duplicated, reviewed and used.
Line 136: client.executeCommand(command, null, out, null);
Line 137: cliOutput = new String(out.toByteArray(), "UTF-8");
Line 138: } catch (Exception e) {
Line 139: String m =
Line 139: String m =
Line 140: String.format("SSH error running command %1$s
'%2$s': %3$s",
Line 141: getParameters().getServerName(),
Line 142: command,
Line 143: e.getMessage());
I don't think you need the getMessage() here, as you inherit the original
exception.
Line 144: log.error(m, e);
Line 145: throw new RuntimeException(e);
Line 146: }
Line 147: return cliOutput;
--
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: 10
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: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
