Shireesh Anjal has posted comments on this change.
Change subject: engine: Gluster server peer list command
......................................................................
Patch Set 3: (5 inline comments)
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/GlusterServerInfo.java
Line 47: @Override
Line 48: public int hashCode() {
Line 49: final int prime = 31;
Line 50: int result = 1;
Line 51: result = prime * result + getUuid().hashCode();
What if uuid is null? Also, it is inconsistent to use getter in case of uuid,
and instance variables in case of hostName and status.
Line 52: result = prime * result + ((hostName == null) ? 0 :
hostName.hashCode());
Line 53: result = prime * result + ((status == null) ? 0 :
status.hashCode());
Line 54: return result;
Line 55: }
Line 60: return false;
Line 61: }
Line 62:
Line 63: GlusterServerInfo host = (GlusterServerInfo) obj;
Line 64: return (getUuid().equals(host.getUuid())
Same here. Could throw NullPointerException if uuid is null.
Line 65: && (GlusterCoreUtil.objectsEqual(hostName,
host.getHostName()))
Line 66: && (GlusterCoreUtil.objectsEqual(status,
host.getStatus())));
Line 67: }
Line 68:
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/GlusterServersListReturnForXmlRpc.java
Line 17: private static final String UUID = "uuid";
Line 18:
Line 19: private static final String PEER_STATUS = "status";
Line 20:
Line 21: public Set<GlusterServerInfo> hostsList = new
HashSet<GlusterServerInfo>();
I would prefer this to be a private variable with a public getter, and
recommend the name to be "servers"
Line 22:
Line 23: public GlusterServersListReturnForXmlRpc(Map<String, Object>
innerMap) {
Line 24: super(innerMap);
Line 25: Object[] temp = (Object[]) innerMap.get(GLUSTER_HOSTS);
Line 21: public Set<GlusterServerInfo> hostsList = new
HashSet<GlusterServerInfo>();
Line 22:
Line 23: public GlusterServersListReturnForXmlRpc(Map<String, Object>
innerMap) {
Line 24: super(innerMap);
Line 25: Object[] temp = (Object[]) innerMap.get(GLUSTER_HOSTS);
I would suggest naming this variable as 'serversArr' instead of 'temp'
Line 26:
Line 27: if (temp != null) {
Line 28: for (int i = 0; i < temp.length; i++) {
Line 29: hostsList.add(prepareHostsEntity((Map<String, Object>)
temp[i]));
Line 36: entity.setHostName(map.get(HOST_NAME).toString());
Line 37: entity.setUuid(new Guid(map.get(UUID).toString()));
Line 38:
Line 39: String status = map.get(PEER_STATUS).toString();
Line 40: if (status.equalsIgnoreCase("CONNECTED")) {
In case the enum names are exactly same as what you expect to receive from
VDSM, you could use PeerStatus.valueOf()
Line 41: entity.setStatus(PeerStatus.CONNECTED);
Line 42: } else if (status.equalsIgnoreCase("DISCONNECTED")) {
Line 43: entity.setStatus(PeerStatus.DISCONNECTED);
Line 44: } else {
--
To view, visit http://gerrit.ovirt.org/7242
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icfacfdc5847e5d871da38d22b6b5efe86ea4d579
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Selvasundaram <[email protected]>
Gerrit-Reviewer: Dhandapani Gopal <[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
