Shireesh Anjal has posted comments on this change.

Change subject: engine: Get Gluster Servers query
......................................................................


Patch Set 10: (2 inline comments)

replies to couple of Alon's comments in-line.

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GetGlusterServersQuery.java
Line 93:             }
Line 94:         }
Line 95:     }
Line 96: 
Line 97:     private void validateFingerprint(SSHClient client) {
In my opinion, shorter a method, the better :) as long as it makes sense. 
Validating the fingerprint is a key piece of code, which is better kept in a 
separate method, and improves readability of code.
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 131:         client.setUser("root");
Line 132:         client.setPassword(getParameters().getRootPassword());
Line 133: 
Line 134:         try {
Line 135:             client.authenticate();
In my opinion, there is no harm in calling authenticate() and executeCommand() 
inside a single method. It's just one level of abstraction, and the story is 
still the same - the four steps are being performed in the correct sequence 
anyway. The log will not be misleading, as long as the message from the 
original exception is also logged.
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 =


--
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 <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Dhandapani Gopal <[email protected]>
Gerrit-Reviewer: Kanagaraj M <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Selvasundaram <[email protected]>
Gerrit-Reviewer: Shireesh Anjal <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to