Sahina Bose has posted comments on this change. Change subject: engine: Monitoring of geo-rep status and statistics ......................................................................
Patch Set 4: (7 comments) Patchset to follow 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 72: refreshGeoRepSessionStatus(cluster, geoRepSessions); Line 73: } Line 74: Line 75: public void refreshGeoRepDataForVolume(final GlusterVolumeEntity volume) { Line 76: VDSGroup cluster = getClusterDao().get(volume.getClusterId()); > Feature support check can be done here itself Keeping it centralized in discoverGeoRepDataInCluster method Line 77: discoverGeoRepDataForVolume(cluster, volume); Line 78: List<GlusterGeoRepSession> geoRepSessions = getGeoRepDao().getGeoRepSessions(volume.getId()); Line 79: refreshGeoRepSessionStatus(cluster, geoRepSessions); Line 80: } Line 91: } Line 92: Line 93: }); Line 94: } Line 95: > I think updatedSessions will be the same list as geoRepSessions. Yes, that's right 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 updatedSessions are null when the geoRepSessions list passed to this method is empty. Moving the check to the start of method 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 discoverGeoRepDataForVol Done Line 114: Line 115: public void discoverGeoRepDataInCluster(VDSGroup cluster) { Line 116: if (!supportsGlusterGeoRepFeature(cluster)) { Line 117: return; Line 130: if (!supportsGlusterGeoRepFeature(cluster)) { Line 131: return; Line 132: } Line 133: Line 134: if (volume == null) { > This check should be done much earlier Done..refactored to remove this method and added null check earlier Line 135: throw new VdcBLLException(VdcBllErrors.GlusterVolumeGeoRepSyncFailed, "No volume information"); Line 136: } Line 137: Line 138: Map<String, GlusterGeoRepSession> sessionsMap = getSessionsFromCLI(cluster, volume.getName()); 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 availa I do think we should add details of these sessions even if engine does not have details of the slave host. As part of update, this fields need to be updated, IMO 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()); http://gerrit.ovirt.org/#/c/34552/4/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/gluster/GlusterVolumeGeoRepStatusForXmlRpc.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/gluster/GlusterVolumeGeoRepStatusForXmlRpc.java: Line 118: public GlusterVolumeGeoRepStatusForXmlRpc(Map<String, Object> innerMap) { Line 119: this(innerMap, true); Line 120: } Line 121: Line 122: public GlusterVolumeGeoRepStatusForXmlRpc(Map<String, Object> innerMap, boolean process) { > can we have a better name for 'process'? How about processSessions or populateSessions? Line 123: super(innerMap); Line 124: if (process && innerMap.containsKey(GEO_REP)) { Line 125: populateSessions((Object[]) innerMap.get(GEO_REP)); Line 126: } -- 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
