Ramesh N has posted comments on this change. Change subject: engine: Monitoring of geo-rep status and statistics ......................................................................
Patch Set 4: (4 comments) http://gerrit.ovirt.org/#/c/34552/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterGeoRepSyncJob.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterGeoRepSyncJob.java: Line 91: } Line 92: Line 93: }); Line 94: } Line 95: I think updatedSessions will be the same list as geoRepSessions. Line 96: List<GlusterGeoRepSession> updatedSessions = ThreadPoolUtil.invokeAll(geoRepSessionCalls); Line 97: if (updatedSessions == null) { Line 98: return; Line 99: } Line 92: Line 93: }); Line 94: } Line 95: Line 96: List<GlusterGeoRepSession> updatedSessions = ThreadPoolUtil.invokeAll(geoRepSessionCalls); When updatedSessions can be null?. Can it be checked in the beginning with geoRepSessions == null? Line 97: if (updatedSessions == null) { Line 98: return; Line 99: } Line 100: for (GlusterGeoRepSession updatedSession : updatedSessions) { Line 109: getGeoRepDao().updateSession(updatedSession); Line 110: updateSessionDetailsInDB(updatedSession); Line 111: } Line 112: } Line 113: } These two methods (discoverGeoRepDataInCluster and discoverGeoRepDataForVolume) does 95% same thing. Can both be merged to one method with volumeName as String param. Null check for volume also can be removed. Line 114: Line 115: public void discoverGeoRepDataInCluster(VDSGroup cluster) { Line 116: if (!supportsGlusterGeoRepFeature(cluster)) { Line 117: return; Line 173: session.getMasterVolumeName()); Line 174: if (Guid.isNullOrEmpty(session.getId())) { Line 175: session.setId(Guid.newGuid()); Line 176: } Line 177: if (session.getSlaveNodeUuid() == null && session.getSlaveVolumeId() == null) { Can we save the geo rep session even if the slaveHost|Volume is not available in the engine?. I think we should not add session details to DB in this case as these information will not be updated for ever. Line 178: // populate ids from the ones that exist in engine Line 179: List<VDS> slaveHosts = getVdsDao().getAllForHostname(session.getSlaveHostName()); Line 180: if (slaveHosts != null && !slaveHosts.isEmpty()) { Line 181: session.setSlaveNodeUuid(slaveHosts.get(0).getId()); -- To view, visit http://gerrit.ovirt.org/34552 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I67e857ab2ce993cded966fb60b361f6962b9a665 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Sahina Bose <[email protected]> Gerrit-Reviewer: Kanagaraj M <[email protected]> Gerrit-Reviewer: Ramesh N <[email protected]> Gerrit-Reviewer: Sahina Bose <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
