Liron Ar has posted comments on this change. Change subject: core: Add CRUD commands and queries for QoS objects (storage) ......................................................................
Patch Set 7: (14 comments) http://gerrit.ovirt.org/#/c/27096/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/QosQueryBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/QosQueryBase.java: Line 6: import org.ovirt.engine.core.dao.qos.QosDao; Line 7: Line 8: Line 9: public abstract class QosQueryBase extends QueriesCommandBase<QosQueryParameterBase> { Line 10: private QosDao<?> qosDao = null; you can remove the = null when you rebase. Line 11: Line 12: public QosQueryBase(QosQueryParameterBase parameters) { Line 13: super(parameters); Line 14: } http://gerrit.ovirt.org/#/c/27096/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/qos/QosCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/qos/QosCommandBase.java: Line 66: } Line 67: return super.getStoragePoolId(); Line 68: } Line 69: Line 70: protected boolean validateParameters() { I'd suggest you to add @NotNull constraint to the parameters members with the relevant groups (DeleteEntity for example) and it'll be validated automatically when you run the command, same as you added for the entity members. also, i'd seperate the parameters by the usecases. Line 71: if (getQos() == null) { Line 72: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_QOS_NOT_FOUND); Line 73: } Line 74: return true; http://gerrit.ovirt.org/#/c/27096/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/qos/RemoveQosCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/qos/RemoveQosCommandBase.java: Line 21: return true; Line 22: } Line 23: Line 24: @Override Line 25: protected boolean validateParameters() { see related comment - http://gerrit.ovirt.org/#/c/27096/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/qos/QosCommandBase.java Line 26: if (getQosId() == null) { Line 27: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_QOS_NOT_FOUND); Line 28: } Line 29: return true; http://gerrit.ovirt.org/#/c/27096/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/qos/RemoveStorageQosCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/qos/RemoveStorageQosCommand.java: Line 4: import org.ovirt.engine.core.bll.validator.QosValidator; Line 5: import org.ovirt.engine.core.common.action.QosParametersBase; Line 6: import org.ovirt.engine.core.common.businessentities.qos.StorageQos; Line 7: import org.ovirt.engine.core.dao.qos.QosDao; Line 8: 1. missing @NonTransactive annotation 2. what about locks? Line 9: public class RemoveStorageQosCommand extends RemoveQosCommandBase<StorageQos, QosValidator<StorageQos>> { Line 10: Line 11: public RemoveStorageQosCommand(QosParametersBase<StorageQos> parameters) { Line 12: super(parameters); Line 17: return getDbFacade().getStorageQosDao(); Line 18: } Line 19: Line 20: @Override Line 21: protected QosValidator<StorageQos> getQosValidator(StorageQos qos) { seems like a leftover as you added StorageQosValidator. also, we can cache it to avoid creating new instance. Line 22: return new QosValidator<StorageQos>(qos) { Line 23: Line 24: @Override Line 25: protected QosDao<StorageQos> getQosDao() { http://gerrit.ovirt.org/#/c/27096/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/qos/UpdateStorageQosCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/qos/UpdateStorageQosCommand.java: Line 5: import org.ovirt.engine.core.bll.validator.QosValidator; Line 6: import org.ovirt.engine.core.common.action.QosParametersBase; Line 7: import org.ovirt.engine.core.common.businessentities.qos.StorageQos; Line 8: import org.ovirt.engine.core.dao.qos.QosDao; Line 9: locks, non transactive.. Line 10: public class UpdateStorageQosCommand extends UpdateQosCommandBase<StorageQos, QosValidator<StorageQos>> { Line 11: Line 12: public UpdateStorageQosCommand(QosParametersBase<StorageQos> parameters) { Line 13: super(parameters); Line 18: return getDbFacade().getStorageQosDao(); Line 19: } Line 20: Line 21: @Override Line 22: protected QosValidator<StorageQos> getQosValidator(StorageQos qos) { seems like a leftover as you added StorageQosValidator. also, we can cache it to avoid creating new instance. Line 23: return new QosValidator<StorageQos>(qos) { Line 24: Line 25: @Override Line 26: protected QosDao<StorageQos> getQosDao() { Line 28: } Line 29: Line 30: @Override Line 31: public ValidationResult allValuesPresent() { Line 32: // TODO: implement should be validated by the checks on the parameters using the javax validation Line 33: return ValidationResult.VALID; Line 34: } Line 35: }; Line 36: } http://gerrit.ovirt.org/#/c/27096/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/QosValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/QosValidator.java: Line 26: } Line 27: Line 28: protected abstract QosDao<T> getQosDao(); Line 29: Line 30: protected List<T> getAllQosInDc() { /s/getAllQosInDc/getAllQosOfType.. to make it clearer, thinking about that i'd also rename the stored procedure as now it's not clear that it'll return qos of specific type Line 31: if (allQos == null) { Line 32: allQos = getQosDao().getAllForStoragePoolId(qos.getStoragePoolId()); Line 33: } Line 34: return allQos; Line 46: /** Line 47: * Verify that the QoS entity has the same DC ID as the one stored in the database. Line 48: */ Line 49: public ValidationResult consistentDataCenter() { Line 50: return (qos != null && (getOldQos() == null || !qos.getStoragePoolId().equals(getOldQos().getStoragePoolId()))) if OldQos is null the message is wrong, imo we can skip this check here. Line 51: ? new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_QOS_INVALID_DC_ID) Line 52: : ValidationResult.VALID; Line 53: } Line 54: Line 55: /** Line 56: * Verify that a name isn't already taken by another QoS entity in the same DC. Line 57: */ Line 58: public ValidationResult nameNotTakenInDc() { Line 59: if (getAllQosInDc() != null) { it should never be null, the dao returns empty list if there are no values. Line 60: for (T iterQos : getAllQosInDc()) { Line 61: if (ObjectUtils.equals(iterQos.getName(), qos.getName())) { Line 62: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_QOS_NAME_EXIST); Line 63: } Line 58: public ValidationResult nameNotTakenInDc() { Line 59: if (getAllQosInDc() != null) { Line 60: for (T iterQos : getAllQosInDc()) { Line 61: if (ObjectUtils.equals(iterQos.getName(), qos.getName())) { Line 62: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_QOS_NAME_EXIST); you should add to the related commans lock by the chosen name, otherwise race condition will allow two objects with the same name to be added. Line 63: } Line 64: } Line 65: } Line 66: return ValidationResult.VALID; Line 68: Line 69: /** Line 70: * Verify that if any capping was specified, that all parameters are present. Line 71: */ Line 72: public abstract ValidationResult allValuesPresent(); it should be validated by the javax validation http://gerrit.ovirt.org/#/c/27096/7/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/QosParametersBase.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/QosParametersBase.java: Line 4: Line 5: import org.ovirt.engine.core.common.businessentities.qos.QosBase; Line 6: import org.ovirt.engine.core.compat.Guid; Line 7: Line 8: public class QosParametersBase<T extends QosBase> extends VdcActionParametersBase { see related suggestions in previous files. Line 9: Line 10: private static final long serialVersionUID = 1304387921254822524L; Line 11: Line 12: @Valid -- To view, visit http://gerrit.ovirt.org/27096 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1a9af59277b5055453159f002f19046c00522ddb Gerrit-PatchSet: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Doron Fediuck <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Kobi Ianko <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Moti Asayag <[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
