Eli Mesika has posted comments on this change. Change subject: gluster: BLL queries for volume snapshots list and count ......................................................................
Patch Set 19: (6 comments) http://gerrit.ovirt.org/#/c/35674/19/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/GlusterVolumeEntity.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/GlusterVolumeEntity.java: Line 67: @NotNull(message = "VALIDATION.GLUSTER.VOLUME.BRICKS.NOT_NULL", groups = {CreateEntity.class, CreateReplicatedVolume.class, CreateStripedVolume.class}) Line 68: @Valid Line 69: private List<GlusterBrickEntity> bricks; Line 70: Line 71: private Integer noOfSnapshots; is there any reason for not using simply an int ? Line 72: Line 73: private Integer snapMaxLimit; Line 74: Line 75: private GlusterStatus status; Line 69: private List<GlusterBrickEntity> bricks; Line 70: Line 71: private Integer noOfSnapshots; Line 72: Line 73: private Integer snapMaxLimit; same Line 74: Line 75: private GlusterStatus status; Line 76: Line 77: // Gluster and NFS are enabled by default Line 306: public Integer getNoOfSnapshots() { Line 307: return this.noOfSnapshots; Line 308: } Line 309: Line 310: public void setNoOfSnapshots(Integer value) { Minor: worth calling that snapshots rather than value in the same manner that you are using limit in the other setter below Line 311: this.noOfSnapshots = value; Line 312: } Line 313: Line 314: public Integer getSnapMaxLimit() { http://gerrit.ovirt.org/#/c/35674/19/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/gluster/GlusterVolumeDaoDbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/gluster/GlusterVolumeDaoDbFacadeImpl.java: Line 244: "GetGlusterTaskByGlusterVolumeGuid", Line 245: glusterAsyncTaskRowMapper, Line 246: createVolumeIdParams(volumeId)); Line 247: Line 248: if (glusterAsyncTasks != null && !glusterAsyncTasks.isEmpty()) { This change seems unrelated .... Line 249: return glusterAsyncTasks.get(0); Line 250: } Line 251: return null; Line 252: } Line 342: dbFacade.getGlusterVolumeSnapshotConfigDao() Line 343: .getConfigByVolumeIdAndName(volume.getClusterId(), Line 344: volume.getId(), Line 345: GlusterConstants.VOLUME_SNAPSHOT_MAX_HARD_LIMIT); Line 346: if (config == null || config.getParamValue() == null || config.getParamValue().equals("")) { I would rely here on the length of the trimmed string, comparing it to 0 ... or use StringUtils.isEmpty Line 347: config = Line 348: dbFacade.getGlusterVolumeSnapshotConfigDao() Line 349: .getConfigByClusterIdAndName(volume.getClusterId(), Line 350: GlusterConstants.VOLUME_SNAPSHOT_MAX_HARD_LIMIT); http://gerrit.ovirt.org/#/c/35674/19/packaging/dbscripts/gluster_volume_snapshot_sp.sql File packaging/dbscripts/gluster_volume_snapshot_sp.sql: Line 137: END LOOP; Line 138: CLOSE v_cur; Line 139: Line 140: DELETE FROM gluster_volume_snapshots Line 141: WHERE snapshot_id in (select * from fnSplitterUuid(v_snapshot_ids)); maybe it worth encapsulating all those count inc/dec in Create or replace FUNCTION UpdateSnapshotCount(v_volume_id UUID, v_num int) RETURNS VOID AS $procedure$ BEGIN UPDATE gluster_volumes SET snapshot_count = snapshot_count + v_num WHERE id = v_volume_id; END; $procedure$ LANGUAGE plpgsql; and then call this from all SPS with PERFORM Line 142: END; $procedure$ Line 143: LANGUAGE plpgsql; Line 144: Line 145: -- To view, visit http://gerrit.ovirt.org/35674 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I00cd2a52c9bd10946b8702d833d6f8f0ebb3f848 Gerrit-PatchSet: 19 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Shubhendu Tripathi <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Kanagaraj M <[email protected]> Gerrit-Reviewer: Sahina Bose <[email protected]> Gerrit-Reviewer: Shubhendu Tripathi <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
