Shubhendu Tripathi has posted comments on this change. Change subject: gluster: BLL command to set volume snapshot config ......................................................................
Patch Set 2: (3 comments) http://gerrit.ovirt.org/#/c/36294/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/UpdateGlusterVolumeSnapshotConfigCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/UpdateGlusterVolumeSnapshotConfigCommand.java: Line 49: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_CLUSTER_IS_NOT_VALID); Line 50: return false; Line 51: } Line 52: Line 53: if (getParameters() == null || getParameters().getConfigParams() == null > getParameters() == null is redundant check as you have already used getPara yes. would change this Line 54: || getParameters().getConfigParams().size() == 0) { Line 55: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_GLUSTER_VOLUME_SNAPSHOT_CONFIG_PARAMS_IS_EMPTY); Line 56: return false; Line 57: } Line 87: } Line 88: Line 89: List<GlusterVolumeSnapshotConfig> configParams = getParameters().getConfigParams(); Line 90: Line 91: // segregate the cluster and volume specific config params > why not just send them as two different lists? Below these maps are used to check if the values are actually changed, and then only they are updated into DB. Line 92: Map<String, GlusterVolumeSnapshotConfig> clusterConfigParams = Line 93: new HashMap<String, GlusterVolumeSnapshotConfig>(); Line 94: Map<String, GlusterVolumeSnapshotConfig> volumeConfigParams = Line 95: new HashMap<String, GlusterVolumeSnapshotConfig>(); Line 106: new ArrayList<GlusterVolumeSnapshotConfig>(); Line 107: for (GlusterVolumeSnapshotConfig cfgParam : clusterConfigParams.values()) { Line 108: GlusterVolumeSnapshotConfig fetchedCfgParam = fetchedClusterConfigParams.get(cfgParam.getParamName()); Line 109: if (!(fetchedCfgParam.getParamValue().equals(cfgParam.getParamValue()))) { Line 110: updatedClusterConfigParams.add(cfgParam); > Why do we need to fetch and update, instead of simply updating it directly? First check if the values are actually changed and then only update into DB. Do you feel we can simply update into DB? Line 111: } Line 112: } Line 113: List<GlusterVolumeSnapshotConfig> updatedVolumeConfigParams = Line 114: new ArrayList<GlusterVolumeSnapshotConfig>(); -- To view, visit http://gerrit.ovirt.org/36294 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I37b6f4dd02ca4258e5e40e7da11f0a657ac6b330 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Shubhendu Tripathi <[email protected]> Gerrit-Reviewer: Kanagaraj M <[email protected]> Gerrit-Reviewer: Ramesh N <[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
