Shubhendu Tripathi 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 ? Nothing specific. Just to be inline with fields like stripeCount etc. 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 As above 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: Will rename the attribute as snapahotsCount and method names accordingly 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 .... some noise due to formatting of source 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 .. done 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 so you mean two SPs for increasing and decreasing the count and then use them? Also in one case we are setting the count to zero. Does that remain as is ? 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 <shtri...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Kanagaraj M <kmayi...@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
