Sahina Bose has posted comments on this change. Change subject: gluster: Sync job for gluster volume snapshots ......................................................................
Patch Set 5: (11 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? 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 volumename parameter? If you skip the volumename, won't the command return information for all volumes in cluster? 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? 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 stopped? Should we then delete from the database? 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 - '{}'", e.getMessage()); log.debug("Exception", e) seems to be the convention followed 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 are introduced. 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? 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 extend from GlusterVolumeOptionEntity? 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? 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 { > You could rename this to GlusterVolumeSnapshotListInfo, since this contains Why is this class needed? Won't List<GlusterVolumeSnapshotEntity> do? 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 feel 5 seconds would be too aggressive. You could change this to 30 secon I agree, even to 1 min? Are we providing a UI option to trigger sync? Line 1425: GlusterRefreshRateSnapshotDiscovery, Line 1426: Line 1427: @TypeConverterAttribute(String.class) Line 1428: @DefaultValueAttribute("AttestationService/resources/PollHosts") -- 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: 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
