Shireesh Anjal has posted comments on this change.
Change subject: engine: Get Gluster Servers query
......................................................................
Patch Set 3: (4 inline comments)
Few in-line comments.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GetGlusterServersQuery.java
Line 20: import org.w3c.dom.NodeList;
Line 21: import org.xml.sax.SAXException;
Line 22:
Line 23: /**
Line 24: * Query to fetch list of gluster servers via ssh using the given
serverName and password.
Please enhance this comment explaining why ssh is being used instead of a vds
command.
Line 25: *
Line 26: */
Line 27: public class GetGlusterServersQuery<P extends
GlusterServersQueryParameters> extends GlusterQueriesCommandBase<P> {
Line 28:
Line 122: if(firstPeer.getNodeType() == Node.ELEMENT_NODE){
Line 123: Element firstHostElement = (Element)firstPeer;
Line 124: int state = getIntValue(firstHostElement, STATE);
Line 125: // Add the server only if the state is 3
Line 126: if (state == 3) {
Please introduce a constant for 3 (PEER_IN_CLUSTER).
Line 127: hostNamesOrIp.add(getTextValue(firstHostElement,
HOST_NAME));
Line 128: }
Line 129: }
Line 130: }
Line 134: return Integer.parseInt(getTextValue(element,tagName));
Line 135: }
Line 136:
Line 137: private String getTextValue(Element element, String tagName) {
Line 138: NodeList hostName = element.getElementsByTagName(tagName);
While the method looks like a generic one fetching value of a given tag, the
variable "hostName" sounds too specific, as if the tagName will always be
HOST_NAME. So you can either change the method prototype to not take a tagName
argument at all, or change the variable name to something more generic, say
"nodes"
Line 139: Element hostNameElement = (Element) hostName.item(0);
Line 140:
Line 141: NodeList hostNameNode = hostNameElement.getChildNodes();
Line 142: return hostNameNode.item(0).getNodeValue().trim();
....................................................
Commit Message
Line 5: CommitDate: 2012-09-03 18:37:55 +0530
Line 6:
Line 7: engine: Get Gluster Servers query
Line 8:
Line 9: - Execute the gluster peer status command via ssh using the given
servername and password
Please add more explanation to make it clear why you need to use ssh as against
a vds command.
Line 10: - Parse the xml output of the command and get the servernames which
state is 3(ie. Peer in Cluster)
Line 11: - Fetch the finger print of the servers and return
Line 12: - Added new Junit test
Line 13:
--
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: 3
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: 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
