Shubhendu Tripathi has posted comments on this change. Change subject: gluster: Sync job for gluster volume snapshots ......................................................................
Patch Set 12: (16 comments) http://gerrit.ovirt.org/#/c/35904/12/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 68: return; Line 69: } Line 70: Line 71: final VDS upServer = getClusterUtils().getRandomUpServer(cluster.getId()); Line 72: if (upServer == null) { > log that sync did not run at this time? done Line 73: return; Line 74: } Line 75: Line 76: try { Line 74: } Line 75: Line 76: try { Line 77: List<GlusterVolumeSnapshotEntity> snapshotsList = Line 78: (ArrayList<GlusterVolumeSnapshotEntity>) runVdsCommand(VDSCommandType.GetGlusterVolumeSnapshotInfo, > How about a check if the command succeeded? Otherwise returnValue is null, Will try doing the same Line 79: new GlusterVolumeSnapshotVDSParameters(upServer.getId(), Line 80: null, Line 81: cluster.getId())).getReturnValue(); Line 82: addOrUpdateSnapshots(cluster.getId(), snapshotsList); Line 75: Line 76: try { Line 77: List<GlusterVolumeSnapshotEntity> snapshotsList = Line 78: (ArrayList<GlusterVolumeSnapshotEntity>) runVdsCommand(VDSCommandType.GetGlusterVolumeSnapshotInfo, Line 79: new GlusterVolumeSnapshotVDSParameters(upServer.getId(), > Ideally the constructor for GlusterVolumeSnapshotVDSParameters should be (s Will do that Line 80: null, Line 81: cluster.getId())).getReturnValue(); Line 82: addOrUpdateSnapshots(cluster.getId(), snapshotsList); Line 83: } catch (VdcBLLException e) { Line 80: null, Line 81: cluster.getId())).getReturnValue(); Line 82: addOrUpdateSnapshots(cluster.getId(), snapshotsList); Line 83: } catch (VdcBLLException e) { Line 84: throw e; > You're throwing an exception from a scheduled method - this will result in I think we can just log and continue in this case Line 85: } Line 86: } Line 87: Line 88: private void refreshSnapshotConfigInCluster(VDSGroup cluster) { Line 95: return; Line 96: } Line 97: Line 98: try { Line 99: GlusterSnapshotConfigInfo configInfo = > same as above done Line 100: (GlusterSnapshotConfigInfo) runVdsCommand(VDSCommandType.GetGlusterVolumeSnapshotConfigInfo, Line 101: new GlusterVolumeSnapshotVDSParameters(upServer.getId(), Line 102: null, Line 103: cluster.getId())).getReturnValue(); Line 109: Line 110: private void addOrUpdateSnapshots(Guid clusterId, List<GlusterVolumeSnapshotEntity> fetchedSnapshots) { Line 111: // form volume-wise map of snapshots Line 112: Map<Guid, List<GlusterVolumeSnapshotEntity>> volumeWiseSnapshots = Line 113: new HashMap<Guid, List<GlusterVolumeSnapshotEntity>>(); > new HashMap<> would do yes. will do that Line 114: for (GlusterVolumeSnapshotEntity snapshot : fetchedSnapshots) { Line 115: if (volumeWiseSnapshots.get(snapshot.getVolumeId()) == null) { Line 116: volumeWiseSnapshots.put(snapshot.getVolumeId(), new ArrayList<GlusterVolumeSnapshotEntity>()); Line 117: } Line 112: Map<Guid, List<GlusterVolumeSnapshotEntity>> volumeWiseSnapshots = Line 113: new HashMap<Guid, List<GlusterVolumeSnapshotEntity>>(); Line 114: for (GlusterVolumeSnapshotEntity snapshot : fetchedSnapshots) { Line 115: if (volumeWiseSnapshots.get(snapshot.getVolumeId()) == null) { Line 116: volumeWiseSnapshots.put(snapshot.getVolumeId(), new ArrayList<GlusterVolumeSnapshotEntity>()); > same as line 113 yes. will do that Line 117: } Line 118: volumeWiseSnapshots.get(snapshot.getVolumeId()).add(snapshot); Line 119: } Line 120: Line 122: List<GlusterVolumeSnapshotEntity> updatedSnapshots = new ArrayList<>(); Line 123: List<GlusterVolumeSnapshotEntity> newlyAddedSnapshots = new ArrayList<>(); Line 124: List<GlusterVolumeSnapshotEntity> deletedSnapshots = new ArrayList<>(); Line 125: Line 126: for (Map.Entry<Guid, List<GlusterVolumeSnapshotEntity>> entry : volumeWiseSnapshots.entrySet()) { > Is there any reason to build a volumeWiseSnapshot map? Can you not do a get Will have to introduce a method getAllByClusterId. Will do that Line 127: List<GlusterVolumeSnapshotEntity> fetchedVolumeSnapshots = entry.getValue(); Line 128: Map<Guid, GlusterVolumeSnapshotEntity> fetchedSnapshotsMap = Line 129: new HashMap<Guid, GlusterVolumeSnapshotEntity>(); Line 130: for (final GlusterVolumeSnapshotEntity fetchedSnapshot : fetchedVolumeSnapshots) { Line 159: } Line 160: } Line 161: Line 162: // update snapshot details Line 163: try (EngineLock lock = acquireEntityLock(clusterId)) { > Why do you need a cluster level lock here? Actually volume level required. Will change that Line 164: saveNewSnapshots(newlyAddedSnapshots); Line 165: updateSnapshots(updatedSnapshots); Line 166: deleteSnapshots(deletedSnapshots); Line 167: } Line 241: paramValue); Line 242: } Line 243: } Line 244: Line 245: private void saveNewSnapshots(List<GlusterVolumeSnapshotEntity> snaphosts) { > typo in argument done Line 246: for (GlusterVolumeSnapshotEntity snapshot : snaphosts) { Line 247: getGlusterVolumeSnapshotDao().save(snapshot); Line 248: } Line 249: } Line 243: } Line 244: Line 245: private void saveNewSnapshots(List<GlusterVolumeSnapshotEntity> snaphosts) { Line 246: for (GlusterVolumeSnapshotEntity snapshot : snaphosts) { Line 247: getGlusterVolumeSnapshotDao().save(snapshot); > saveAllInBatch? done Line 248: } Line 249: } Line 250: Line 251: private void updateSnapshots(List<GlusterVolumeSnapshotEntity> snapshots) { Line 249: } Line 250: Line 251: private void updateSnapshots(List<GlusterVolumeSnapshotEntity> snapshots) { Line 252: for (GlusterVolumeSnapshotEntity snapshot : snapshots) { Line 253: getGlusterVolumeSnapshotDao().updateSnapshotStatus(snapshot.getId(), snapshot.getStatus()); > same as above done Line 254: } Line 255: } Line 256: Line 257: private void deleteSnapshots(List<GlusterVolumeSnapshotEntity> snaphosts) { Line 276: protected GlusterVolumeSnapshotConfigDao getGlusterVolumeSnapshotConfigDao() { Line 277: return DbFacade.getInstance().getGlusterVolumeSnapshotConfigDao(); Line 278: } Line 279: Line 280: protected EngineLock acquireEntityLock(Guid entityId) { > As Yair commented, why not use super class lock method? Will do that. The signature of the method looks little confusing as if its getting lock for cluster. But actually we can get lock for volume or cluster using the same method. Line 281: return GlusterUtil.getInstance().acquireGlusterLockWait(entityId); Line 282: } http://gerrit.ovirt.org/#/c/35904/12/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 8: public class GlusterSnapshotConfigInfo implements Serializable { Line 9: private static final long serialVersionUID = -768822766895441288L; Line 10: Line 11: private Map<String, String> clusterConfigOptions; Line 12: private Map<String, Map<String, String>> volumeConfigOptions; > what's the key here? <volumeId , <configKey, configValue>> ? Its like <volumeName, <configKey, configValue>> Line 13: Line 14: public Map<String, String> getClusterConfigOptions() { Line 15: return this.clusterConfigOptions; Line 16: } http://gerrit.ovirt.org/#/c/35904/12/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 1417: GlusterVolumeSnapshotSupported, Line 1418: Line 1419: @TypeConverterAttribute(Integer.class) Line 1420: @DefaultValueAttribute("3600") Line 1421: GlusterRefreshRateGeoRepDiscovery, > is this change required? Oh sorry. not required. Snapshot refresh rate is already 300 sec. Line 1422: Line 1423: @TypeConverterAttribute(Integer.class) Line 1424: @DefaultValueAttribute("300") Line 1425: GlusterRefreshRateSnapshotDiscovery, http://gerrit.ovirt.org/#/c/35904/12/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/gluster/GlusterVolumeSnapshotConfigReturnForXmlRpc.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/gluster/GlusterVolumeSnapshotConfigReturnForXmlRpc.java: Line 44: Map<String, String> volConfig = new HashMap<String, String>(); Line 45: Line 46: for (Map.Entry<String, Object> config : fetchedVolumeConfig.entrySet()) { Line 47: String value = (String) config.getValue(); Line 48: volConfig.put(config.getKey(), value == null ? "" : value); > why not null as value? Should not be an issue. Will test that. Line 49: } Line 50: Line 51: volumeConfigs.put(entry.getKey(), volConfig); Line 52: } -- 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: 12 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
