Shubhendu Tripathi has posted comments on this change. Change subject: gluster: BLL command to set volume snapshot config ......................................................................
Patch Set 8: (5 comments) http://gerrit.ovirt.org/#/c/36294/8/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 55: return false; Line 56: } Line 57: Line 58: for (GlusterVolumeSnapshotConfig param : getParameters().getConfigParams()) { Line 59: if (param.getParamValue() == null || param.getParamValue().equals("")) { > StringUtils.isBlank ? done Line 60: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_GLUSTER_VOLUME_SNAPSHOT_CONFIG_PARAM_VALUE_IS_EMPTY); Line 61: return false; Line 62: } Line 63: } Line 99: // form the final list of updated config params Line 100: List<GlusterVolumeSnapshotConfig> updatedClusterConfigParams = new ArrayList<>(); Line 101: for (GlusterVolumeSnapshotConfig cfgParam : clusterConfigParams.values()) { Line 102: GlusterVolumeSnapshotConfig fetchedCfgParam = fetchedClusterConfigParams.get(cfgParam.getParamName()); Line 103: if (fetchedCfgParam != null && !(fetchedCfgParam.getParamValue().equals(cfgParam.getParamValue()))) { > What about for new parameters added ? (which are not yet stored in DB) The list of parameters is actually defined here so only updated values we need to set. Line 104: updatedClusterConfigParams.add(cfgParam); Line 105: } Line 106: } Line 107: List<GlusterVolumeSnapshotConfig> updatedVolumeConfigParams = new ArrayList<>(); Line 122: VDSReturnValue retVal = runVdsCommand(VDSCommandType.SetGlusterVolumeSnapshotConfig, Line 123: new GlusterVolumeSnapshotSetConfigVDSParameters(upServer.getId(), config)); Line 124: Line 125: if (!retVal.getSucceeded()) { Line 126: handleVdsError(AuditLogType.GLUSTER_VOLUME_SNAPSHOT_CONFIG_UPDATE_FAILED, retVal.getVdsError() > Would this command fail if even one of the configs failed? How about loggin Should we just log the failed one and continue with others ?? Line 127: .getMessage()); Line 128: setSucceeded(false); Line 129: return; Line 130: } else { Line 151: protected Map<String, Pair<String, String>> getExclusiveLocks() { Line 152: Guid clusterId = getParameters().getConfigParams().get(0).getClusterId(); Line 153: if (!isInternalExecution()) { Line 154: return Collections.singletonMap(clusterId.toString(), Line 155: LockMessagesMatchUtil.makeLockingPair(LockingGroup.GLUSTER, > Same comment as before - please check the locking group to use done Line 156: VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED)); Line 157: } Line 158: return null; Line 159: } http://gerrit.ovirt.org/#/c/36294/8/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/gluster/UpdateGlusterVolumeSnapshotConfigParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/gluster/UpdateGlusterVolumeSnapshotConfigParameters.java: Line 9: public class UpdateGlusterVolumeSnapshotConfigParameters extends VdcActionParametersBase { Line 10: private static final long serialVersionUID = 2015321730118872977L; Line 11: Line 12: private Guid clusterId; Line 13: // Nullable, so not extending the class from GlusterVolumeParameters > I don't understand. Can you not pass null value to volumeId even if you ext I remember it fails in canDoAction if null passed for volumeId. But here in our case its intentional null for cluster configurations. Line 14: private Guid volumeId; Line 15: private List<GlusterVolumeSnapshotConfig> configParams; Line 16: Line 17: public UpdateGlusterVolumeSnapshotConfigParameters() { -- 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: 8 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
