anmolbabu has posted comments on this change. Change subject: engine : Geo-rep config Vds Commands ......................................................................
Patch Set 5: (3 comments) http://gerrit.ovirt.org/#/c/35972/5/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/gluster/GlusterVolumeGeoRepSessionVDSParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/gluster/GlusterVolumeGeoRepSessionVDSParameters.java: Line 3: import org.ovirt.engine.core.compat.Guid; Line 4: Line 5: public class GlusterVolumeGeoRepSessionVDSParameters extends GlusterVolumeVDSParameters { Line 6: Line 7: private Guid sessionId; > Is this required? VDSM does not know of geo-rep sessionId, right? Done. As there is no session Id in glusterfs, there is no session Id returned along with the config list. I thought of setting it in Vds command before returning the result but since updating the db is done by sync job.May be sync job can do this on per session basis. Line 8: private String slaveHost; Line 9: private String slaveVolume; Line 10: private Boolean force = false; Line 11: http://gerrit.ovirt.org/#/c/35972/5/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/gluster/GetGlusterVolumeGeoRepConfigListVDSCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/gluster/GetGlusterVolumeGeoRepConfigListVDSCommand.java: Line 24: protected StatusForXmlRpc getReturnStatus() { Line 25: return result.mStatus; Line 26: } Line 27: Line 28: public Map<String, GlusterGeoRepSessionConfiguration> prepareMapOfExistingConfigs(List<GlusterGeoRepSessionConfiguration> existingConfigs) { > Do we need this method to be public? Done. This method no longer required. Removing it. Line 29: Map<String, GlusterGeoRepSessionConfiguration> resultantMap = new HashMap<>(); Line 30: if (existingConfigs != null) { Line 31: for (GlusterGeoRepSessionConfiguration config : existingConfigs) { Line 32: resultantMap.put(config.getKey(), config); Line 47: session.getSlaveVolumeName()); Line 48: proceedProxyReturnValue(); Line 49: Line 50: List<GlusterGeoRepSessionConfiguration> sessionConfigs = result.getSessionConfig(); Line 51: if (!existingSessionConfigs.containsAll(sessionConfigs)) { > Do you want to update the database while retrieving this data here? or leav Done.My actual intention was to do it this way : On click of options from geo-rep sub tab if the config data hasn't been synced into db, fetch it and then pass it onto db and also to bll and hence UI. (The query patch doesn't look like this). But I now feel that leaving it be handled by sync job itself is a better option as we can have this data being fetched/synced as soon as a geo-rep session is created. This being the scenario, I now strongly concur with you in leaving this to sync job. And hence will update these changes as a part of next patch set. Line 52: for (GlusterGeoRepSessionConfiguration geoRepSessionConfig : sessionConfigs) { Line 53: if (!existingSessionConfigs.contains(geoRepSessionConfig)) { Line 54: if (prepareMapOfExistingConfigs(existingSessionConfigs).containsKey(geoRepSessionConfig.getKey())) { Line 55: geoRepDao.updateConfig(geoRepSessionConfig); -- To view, visit http://gerrit.ovirt.org/35972 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I558ed18b94faae9d4b824c9de1fedda25ffc3f09 Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: anmolbabu <anb...@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
