anmolbabu has posted comments on this change. Change subject: engine : Query to fetch list of volumes eligible for geo replication ......................................................................
Patch Set 14: (11 comments) http://gerrit.ovirt.org/#/c/33845/14/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/CheckEligibilityOfVolumesForGeoRepSessionCreationQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/CheckEligibilityOfVolumesForGeoRepSessionCreationQuery.java: Line 17: } Line 18: Line 19: @Override Line 20: protected void executeQueryCommand() { Line 21: getQueryReturnValue().setReturnValue(processVolumesInCurrentCluster(getGlusterVolumeDao().getById(getParameters().getId()), getGlusterVolumeDao().getById(getParameters().getSlaveVolumeId()), prepareSessionMap(getGeoRepDao().getAllSessions()))); > why pass the prepareSessionMap here? why not get sessions in the processVol Done Line 22: } Line 23: Line 24: protected List<Guid> prepareSessionMap(List<GlusterGeoRepSession> existingSessions) { Line 25: List<Guid> sessionSlavesIds = new ArrayList<Guid>(); Line 20: protected void executeQueryCommand() { Line 21: getQueryReturnValue().setReturnValue(processVolumesInCurrentCluster(getGlusterVolumeDao().getById(getParameters().getId()), getGlusterVolumeDao().getById(getParameters().getSlaveVolumeId()), prepareSessionMap(getGeoRepDao().getAllSessions()))); Line 22: } Line 23: Line 24: protected List<Guid> prepareSessionMap(List<GlusterGeoRepSession> existingSessions) { > prepareSessionMap returns a list? consider renaming As per your previous comment ,I'll remove this and do this inside processVolumesInCurrentCluster for which I wrote this method Line 25: List<Guid> sessionSlavesIds = new ArrayList<Guid>(); Line 26: for(GlusterGeoRepSession currentSession : existingSessions) { Line 27: sessionSlavesIds.add(currentSession.getSlaveVolumeId()); Line 28: } http://gerrit.ovirt.org/#/c/33845/14/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GetAllVolumesQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GetAllVolumesQuery.java: Line 22: } Line 23: Line 24: private Set<GlusterVolumeEntity> getAllVolumes(Guid masterVolumeId) { Line 25: List<VDSGroup> clusters = getVdsGroupDao().getAll(); Line 26: Set<GlusterVolumeEntity> volumes = new HashSet<GlusterVolumeEntity>(); > why not just use glusterVolumeDao.getAllWithQuery? in which case you can pa Done Line 27: if(clusters != null) { Line 28: for(VDSGroup currentCluster : clusters) { Line 29: volumes.addAll(getGlusterVolumeDao().getByClusterId(currentCluster.getId())); Line 30: } http://gerrit.ovirt.org/#/c/33845/14/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GetGlusterGeoReplicationEligibleVolumesQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GetGlusterGeoReplicationEligibleVolumesQuery.java: Line 26: List<GlusterVolumeEntity> allGlusterVolumesWithMasterCompatibleVersion = getAllGlusterVolumesWithMasterCompatibleVersion(masterVolumeId); Line 27: CheckEligibilityOfVolumesForGeoRepSessionCreationQuery<CheckGlusterVolumeGeoRepEligibilityParameters> eligibilityQuery = new CheckEligibilityOfVolumesForGeoRepSessionCreationQuery<CheckGlusterVolumeGeoRepEligibilityParameters>(new CheckGlusterVolumeGeoRepEligibilityParameters(masterVolumeId, null)); Line 28: List<Guid> sessionSlavesIds = eligibilityQuery.prepareSessionMap(getGeoRepDao().getAllSessions()); Line 29: for(GlusterVolumeEntity currentVolume : allGlusterVolumesWithMasterCompatibleVersion) { Line 30: List<GEO_REP_NON_ELIGIBILITY_REASON> nonEligibilityResons = eligibilityQuery.processVolumesInCurrentCluster(masterVolume, currentVolume, sessionSlavesIds); > You could move the processVolumes method into a GlusterGeoRepUtils and call Done Line 31: if(nonEligibilityResons == null || nonEligibilityResons.size() == 0) { Line 32: eligibleVolumes.add(currentVolume); Line 33: } Line 34: } Line 40: VDSGroup masterCluster = getVdsGroupDao().get(masterVolume.getClusterId()); Line 41: List<VDSGroup> clusters = getVdsGroupDao().getClustersByServiceAndCompatibilityVersion(true, false, masterCluster.getcompatibility_version().getValue()); Line 42: List<GlusterVolumeEntity> volumes = new ArrayList<GlusterVolumeEntity>(); Line 43: if(clusters != null) { Line 44: for(VDSGroup currentCluster : clusters) { > can you not filter out cluster where the master volume belongs too? Done Line 45: volumes.addAll(getGlusterVolumeDao().getByClusterId(currentCluster.getId())); Line 46: } Line 47: } Line 48: volumes.remove(masterVolume); Line 44: for(VDSGroup currentCluster : clusters) { Line 45: volumes.addAll(getGlusterVolumeDao().getByClusterId(currentCluster.getId())); Line 46: } Line 47: } Line 48: volumes.remove(masterVolume); > if you follow above comment, this is not required. Done Line 49: return volumes; Line 50: } http://gerrit.ovirt.org/#/c/33845/14/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/GlusterGeoRepEnums.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/GlusterGeoRepEnums.java: Line 1: package org.ovirt.engine.core.common.businessentities.gluster; Line 2: Line 3: public class GlusterGeoRepEnums { Line 4: public static enum GeoRepEligibility { > Do you need an enum for this? true or false would do? I wrote this for the sake of the huge Map<GlusterGeoRepEligibility,Map<String, Map ...>>(One-to-many reduced to one-to-one) that I used earlier Line 5: ELIGIBLE, Line 6: NOT_ELIGIBLE; Line 7: } Line 8: Line 5: ELIGIBLE, Line 6: NOT_ELIGIBLE; Line 7: } Line 8: Line 9: public static enum GEO_REP_NON_ELIGIBILITY_REASON { > Does this need to be a static enum? I'll change it Line 10: SLAVE_AND_MASTER_VOLUMES_IN_SAME_CLUSTER, Line 11: SLAVE_VOLUME_SIZE_NOT_GREATER_THAN_OR_EQUAL_TO_MASTER_VOLUME_SIZE, Line 12: SLAVE_CLUSTER_AND_MASTER_CLUSTER_COMPATIBILITY_VERSIONS_DO_NOT_MATCH, Line 13: SLAVE_VOLUME_ALREADY_SLAVE_OF_ANOTHER_GEO_REP_SESSION, http://gerrit.ovirt.org/#/c/33845/14/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/VdcQueryType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/VdcQueryType.java: Line 333: GetGlusterVolumeRebalanceStatus, Line 334: GetGlusterVolumeGeoRepSessions, Line 335: GetGlusterVolumeRemoveBricksStatus, Line 336: GetGlusterVolumeByTaskId, Line 337: CheckEligibilityOfVolumesForGeoRepSessionCreation, > CheckEligibility or GetEligibilityofVolumeForGeoRepSession ? Done Line 338: GetAllVolumes, Line 339: GetGlusterGeoReplicationEligibleVolumes, Line 340: Line 341: GetDefaultConfigurationVersion(VdcQueryAuthType.User), http://gerrit.ovirt.org/#/c/33845/14/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/gluster/CheckGlusterVolumeGeoRepEligibilityParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/gluster/CheckGlusterVolumeGeoRepEligibilityParameters.java: Line 3: import org.ovirt.engine.core.common.queries.IdQueryParameters; Line 4: import org.ovirt.engine.core.compat.Guid; Line 5: Line 6: Line 7: public class CheckGlusterVolumeGeoRepEligibilityParameters extends IdQueryParameters { > GlusterVolumeGeoRepEligibilityParameters ? Done Line 8: Line 9: private static final long serialVersionUID = 1L; Line 10: Line 11: private Guid slaveVolumeId; http://gerrit.ovirt.org/#/c/33845/14/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsGroupDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsGroupDAODbFacadeImpl.java: Line 345: @Override Line 346: public List<VDSGroup> getClustersByServiceAndCompatibilityVersion(boolean glusterService, Line 347: boolean virtService, Line 348: String compatibilityVersion) { Line 349: return getCallsHandler().executeReadList("GetVdsGroupsByServiceAndCompatibilityVersion", VdsGroupRowMapper.instance, getCustomMapSqlParameterSource().addValue("gluster_service", glusterService).addValue("virt_service", virtService).addValue("compatibility_version", compatibilityVersion)); > Please format correctly Done Line 350: } -- To view, visit http://gerrit.ovirt.org/33845 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0fc3ecb15535181f1ca2a8780461cb89788a3f41 Gerrit-PatchSet: 14 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: anmolbabu <anb...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: anmolbabu <anb...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
