Shireesh Anjal has posted comments on this change.
Change subject: engine: Get Gluster Servers query
......................................................................
Patch Set 10: (2 inline comments)
....................................................
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) {
Yes - just "validateFingerprint()" in place of the above lines makes it far
more readable.
Agree with throwing InvalidKeyException or AccessControlException.
GeneralSecurityException is not suitable here as it is not a RuntimeException.
The base class (QueriesCommandBase) handles RuntimeExceptions properly.
Also agree with no need for logging the exceptions, as the QueriesCommandBase
logs all RuntimeExceptions.
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();
There could be bigger blocks that contain other smaller blocks.
try {
connect
validate
authenticateAndExecute
} ...
Looks quite similar to what you have suggested, and hence I don't see it as a
big issue.
You point about having a try-catch block around all four steps is a good one
and negates my claim that an additional try-catch block is required, albeit
with a "throws Exception" on each method being called.
On a different note, It would be nice if SshClient methods always threw
exceptions that derive from RuntimeException. Most of the "base" classes in the
framework (e.g. QueriesCommandBase) handle RuntimeExceptions gracefully. That
way, we won't have to add either a "throws Exception" clause to methods, or
catch the exception, wrap it in a RuntimeException and throw it again. The code
will look *really* clean. As you've rightly pointed out, in most cases there is
no special handling required in the "business logic" classes, and such
exceptions should be normally handled at framework level.
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
