Sahina Bose 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? 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, possible NPE further down. 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 (serverId, clusterId, volumeId) , no? 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 a JobExecutionException. Is that what's required? 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 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 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 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 getAllByClusterId to return all stored snapshots in cluster? 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? 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 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? 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 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? 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>> ? 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? 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? 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
