Shubhendu Tripathi has posted comments on this change. Change subject: gluster: Sync job for gluster volume snapshots ......................................................................
Patch Set 5: (12 comments) http://gerrit.ovirt.org/#/c/35904/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterSnapshotSyncJob.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterSnapshotSyncJob.java: Line 67: refreshSnapshotConfigInCluster(cluster, false); Line 68: } Line 69: } Line 70: Line 71: private void refreshSnapshotsInCluster(VDSGroup cluster, boolean throwError) { > when would throwError be true? Actually its not required as never passing false. Might be removed. Line 72: if (!supportsGlusterSnapshotFeature(cluster)) { Line 73: return; Line 74: } Line 75: Line 73: return; Line 74: } Line 75: Line 76: final VDS upServer = getClusterUtils().getRandomUpServer(cluster.getId()); Line 77: List<GlusterVolumeEntity> volumes = getGlusterVolumeDao().getByClusterId(cluster.getId()); > Is it required to iterate through all volumes and execute this command with yes thats a valid point. Will try to use the same way. Line 78: Line 79: if (volumes != null && volumes.size() > 0) { Line 80: List<Callable<Pair<GlusterVolumeEntity, VDSReturnValue>>> taskList = Line 81: new ArrayList<Callable<Pair<GlusterVolumeEntity, VDSReturnValue>>>(); Line 149: List<GlusterVolumeSnapshotEntity> newlyAddedSnapshots = new ArrayList<GlusterVolumeSnapshotEntity>(); Line 150: List<GlusterVolumeSnapshotEntity> deletedSnapshots = new ArrayList<GlusterVolumeSnapshotEntity>(); Line 151: Line 152: for (final GlusterVolumeSnapshotEntity fetchedSnapshot : fetchedSnapshots) { Line 153: GlusterVolumeSnapshotEntity existingSnapshot = existingSnapshotsMap.get(fetchedSnapshot.getId()); > Does gluster maintain a UUID for snapshot? yes. while creation gluster returns a UUID and the same value we store in engine DB. Line 154: Line 155: if (existingSnapshot != null) { Line 156: existingSnapshot.setStatus(fetchedSnapshot.getStatus()); Line 157: updatedSnapshots.add(existingSnapshot); Line 161: } Line 162: Line 163: for (final GlusterVolumeSnapshotEntity existingSnapshot : existingSnapshots) { Line 164: if (fetchedSnapshotsMap.get(existingSnapshot.getId()) == null) { Line 165: deletedSnapshots.add(existingSnapshot); > Could it be possible that fetchedSnapshots are empty as the volume is stopp Even if the volume is stopped, the snapshots list does return the values. So i feel its fine to have the snapshots in DB. Line 166: } Line 167: } Line 168: Line 169: // update snapshot details Line 173: deleteSnapshots(deletedSnapshots); Line 174: } Line 175: } Line 176: } catch (Exception e) { Line 177: log.error("Exception in sync", e); > log.error("Exception occured while adding/updating snapshots from CLI - '{} Done. will change accordingly. Line 178: throw new VdcBLLException(VdcBllErrors.GlusterSnapshotInfoFailedException, e.getLocalizedMessage()); Line 179: } Line 180: } Line 181: Line 183: try (EngineLock lock = acquireEntityLock(clusterId)) { Line 184: addOrUpdateClusterConfig(clusterId, "snap-max-hard-limit", configInfo.getSystemSnapMaxHardLimit() Line 185: .toString()); Line 186: addOrUpdateClusterConfig(clusterId, "snap-max-soft-limit", configInfo.getSystemSoftLimitPcnt()); Line 187: addOrUpdateClusterConfig(clusterId, "auto-delete", configInfo.isAutoDeleteEnabled() ? "enable" : "disable"); > Can we avoid hard-coding the config options? Will not work when new options The issue is that the option names are returned as different XML tags from gluster. Better we can have a literals defined in one place and use them everywhere. What you suggest? Line 188: } Line 189: Line 190: List<GlusterSnapshotConfigInfo.VolumeSnapshotConfigInfo> volumeConfigs = configInfo.getVolumeConfigList(); Line 191: for (GlusterSnapshotConfigInfo.VolumeSnapshotConfigInfo volumeConfig : volumeConfigs) { http://gerrit.ovirt.org/#/c/35904/5/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/gluster/GlusterGeoRepSyncJobTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/gluster/GlusterGeoRepSyncJobTest.java: Line 63: mockConfig(ConfigValues.GlusterGeoReplicationEnabled, Version.v3_5.toString(), true), Line 64: mockConfig(ConfigValues.GlusterGeoReplicationEnabled, Version.v3_4.toString(), false), Line 65: mockConfig(ConfigValues.DefaultMinThreadPoolSize, 10), Line 66: mockConfig(ConfigValues.DefaultMaxThreadPoolSize, 20), Line 67: mockConfig(ConfigValues.DefaultMaxThreadWaitQueueSize, 10) > Is this change required in this patch? Not required in this patch. Not sure why it shows as diff. Will check and correct. Line 68: ); Line 69: Line 70: @Before Line 71: public void init() { http://gerrit.ovirt.org/#/c/35904/5/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/GlusterSnapshotConfigInfo.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/GlusterSnapshotConfigInfo.java: Line 12: Line 13: private Guid clusterId; Line 14: private Integer systemSnapMaxHardLimit; Line 15: private String systemSoftLimitPcnt; Line 16: private Boolean autoDeleteEnabled; > Not key value Map of options? Similar to GlusterVolumeOptionEntity? Can you GlusterVolumeEntity represents one volume option at a time. May be we can think of using a HashMap of options here. Line 17: private List<VolumeSnapshotConfigInfo> volumeConfigList; Line 18: Line 19: public Guid getClusterId() { Line 20: return clusterId; Line 13: private Guid clusterId; Line 14: private Integer systemSnapMaxHardLimit; Line 15: private String systemSoftLimitPcnt; Line 16: private Boolean autoDeleteEnabled; Line 17: private List<VolumeSnapshotConfigInfo> volumeConfigList; > Is this required? This represents volume specific configurations, whereas individual fields represent cluster specific configurations. Line 18: Line 19: public Guid getClusterId() { Line 20: return clusterId; Line 21: } http://gerrit.ovirt.org/#/c/35904/5/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/GlusterVolumeSnapshotInfo.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/GlusterVolumeSnapshotInfo.java: Line 5: Line 6: import org.ovirt.engine.core.common.utils.ObjectUtils; Line 7: import org.ovirt.engine.core.compat.Guid; Line 8: Line 9: public class GlusterVolumeSnapshotInfo implements Serializable { > Why is this class needed? Won't List<GlusterVolumeSnapshotEntity> do? As suggested by Kanagaraj, this might not be required only. Will check and try to remove this. Line 10: private static final long serialVersionUID = -768822766895441199L; Line 11: Line 12: private Guid volumeId; Line 13: private List<GlusterVolumeSnapshotEntity> snapshots; http://gerrit.ovirt.org/#/c/35904/5/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java: Line 1420: @DefaultValueAttribute("3600") Line 1421: GlusterRefreshRateGeoRepDiscovery, Line 1422: Line 1423: @TypeConverterAttribute(Integer.class) Line 1424: @DefaultValueAttribute("5") > I agree, even to 1 min? Are we providing a UI option to trigger sync? 5 sec was just for a testing purpose. May be 1 minute would be good value. Currently there is no manual sync available. Sahina, should we introduce one for the same? Line 1425: GlusterRefreshRateSnapshotDiscovery, Line 1426: Line 1427: @TypeConverterAttribute(String.class) Line 1428: @DefaultValueAttribute("AttestationService/resources/PollHosts") http://gerrit.ovirt.org/#/c/35904/5/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/gluster/GlusterVolumeSnapshotInfoVDSParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/gluster/GlusterVolumeSnapshotInfoVDSParameters.java: Line 8: public GlusterVolumeSnapshotInfoVDSParameters() { Line 9: } Line 10: Line 11: public GlusterVolumeSnapshotInfoVDSParameters(Guid clusterId, Guid serverId, String volumeName, String snapshotName) { Line 12: super(serverId, volumeName, snapshotName); > do we need snapshot name here? Actually its not required as we are always getting info for a volume. Will try removing this. Line 13: this.clusterId = clusterId; Line 14: } Line 15: Line 16: public Guid getClusterId() { -- To view, visit http://gerrit.ovirt.org/35904 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7b7bf79b72fc5680dab301b290e7aa860d5c714d Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Shubhendu Tripathi <shtri...@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: Yair Zaslavsky <yzasl...@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
