Alon Bar-Lev 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) {
Far more readable?
The following statement makes it less readable?
if (!getParameters().getFingerprint().equals(getFingerprint(client))) {
throw new RuntimeException(
String.format(
"SSH Fingerprint of server '%1$s' did not match expected fingerprint
'%2$s'",
getParameters().getServerName()
getParameters().getFingerprint();
)
)
}
Notice I removed the log, as the exception as it is enough to through the
exception and catch this at command level, so there is no need for the extra
errMsg variable.
A single trivial if statement (not that we have a complex condition) or calling
a function is the same in this case.
I would also used InvalidKeyException, AccessControlException or
GeneralSecurityException
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();
Thank you,
My view is that software is like any engineering profession... we build
software using building blocks.
Using SSH within the entire application can have the same structure, built
using the same building blocks.
This way when engineer face an issue at one point he can easily handle an issue
at another point.
There is no sense in manufacturing a new building build without good reason, or
create a different building at different locations for same usage.
The building blocks are:
connect
validate
authenticate
execute
disconnect
There is no need to invent a new bock, just because we don't like to add try.
The problem is different... there is no actual reason to catch exception within
code as we can:
try {
connect
validate
authenticate
execute
}
catch (Exception e) {
// why do we care why exactly do we failed?
// the task at hand failed, this is what important
// we have no failure specific handling anyway
}
finally {
disconnect
}
Code will be much shorter, easier to read, will work the same as now, and be
similar to other similar usages.
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 <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
