Shireesh Anjal has posted comments on this change.
Change subject: engine: Get Gluster Servers query
......................................................................
Patch Set 6: (8 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GetGlusterServersQuery.java
Line 40: private static final String HOST_NAME = "hostname";
Line 41: private static final String STATE = "state";
Line 42: private static final int PEER_IN_CLUSTER = 3;
Line 43: private String host;
Line 44: private int port = 22;
Should this be static final as well?
Line 45:
Line 46: List<String> hostNamesOrIp = new ArrayList<String>();
Line 47: private SSHClient client;
Line 48: private Map<String, String> serverFingerprint = new
HashMap<String, String>();
Line 44: private int port = 22;
Line 45:
Line 46: List<String> hostNamesOrIp = new ArrayList<String>();
Line 47: private SSHClient client;
Line 48: private Map<String, String> serverFingerprint = new
HashMap<String, String>();
How about renaming this to serverFingerprintMap or just fingerprints?
Line 49:
Line 50: public GetGlusterServersQuery(P parameters) {
Line 51: super(parameters);
Line 52: }
Line 72: log.errorFormat("There is problem in parsing xml."
+ e1.getMessage());
Line 73: } catch (SAXException e2) {
Line 74: log.errorFormat("Error in parsing xml." +
e2.getMessage());
Line 75: } catch (IOException e3) {
Line 76: log.errorFormat("There is problem in reading the
file." + e3.getMessage());
This means that exception during xml parsing will be logged, and the map
containing a single element (for the current server) will be returned. This is
wrong. I think you should throw an exception in all error conditions, except
the ones while fetching fingerprints.
Line 77: }
Line 78: // Add current server finger print also in the map
Line 79: serverFingerprint.put(getParameters().getServerName(),
getFingerprint(getParameters().getServerName()));
Line 80:
Line 104: client.authenticate();
Line 105: connectStatus = true;
Line 106: } catch (Exception e) {
Line 107: log.debug(
Line 108: String.format("Could not connect to server %1$s:
%2$s", this.host, e.getMessage()));
Throw exception from here as well.
Line 109: }
Line 110: return connectStatus;
Line 111: }
Line 112:
Line 150:
Line 151: private String getFingerprint(String hostName) {
Line 152: connect(hostName);
Line 153:
Line 154: String fingerprint = "Unknown";
I think the fingerprint can be initialized to null. Client would understand
that if fingerprint is null, it means there was an error when trying to fetch
it.
Line 155: try {
Line 156: PublicKey serverKey = this.client.getServerKey();
Line 157: if (serverKey == null) {
Line 158: log.error("Unable to get host key");
Line 156: PublicKey serverKey = this.client.getServerKey();
Line 157: if (serverKey == null) {
Line 158: log.error("Unable to get host key");
Line 159: } else {
Line 160: String tmpFingerprint =
OpenSSHUtils.getKeyFingerprintString(serverKey);
Once you initialize fingerprint as null, you can directly assign the
fingerprint to it - tmpFingerprint is not required.
Line 161:
Line 162: if (tmpFingerprint == null) {
Line 163: log.error("Unable to get host fingerprint");
Line 164: }
Line 166: fingerprint = tmpFingerprint;
Line 167: }
Line 168: }
Line 169:
Line 170: } catch (Throwable e) {
I don't think any of the code inside the try block can throw an exception. This
try-catch block may not be required.
Line 171: log.errorFormat("Could not fetch fingerprint of server
{0} with message: {1}",
Line 172: hostName, ExceptionUtils.getMessage(e));
Line 173: }
Line 174: return fingerprint;
Line 173: }
Line 174: return fingerprint;
Line 175: }
Line 176:
Line 177: private void parseHostName(NodeList listOfPeers) {
extractServers sounds better to me.
Line 178: for (int i = 0; i < listOfPeers.getLength(); i++) {
Line 179: Node firstPeer = listOfPeers.item(i);
Line 180: if (firstPeer.getNodeType() == Node.ELEMENT_NODE) {
Line 181: Element firstHostElement = (Element) firstPeer;
--
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: 6
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
