Gilad Chaplik has posted comments on this change. Change subject: core: Add CRUD commands and queries for QoS objects (storage) ......................................................................
Patch Set 7: (13 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. sure. 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 t Can be done in a later patch. matter of coding style (no time...) 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/m hope you can let this one slide... 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 1. dealing with transactions is irrelevant here, no vdsm calls, only db query. 2. afaik, I should rely on db for concurrency. 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. > seems like a leftover as you added StorageQosValidator Done. > also, we can cache it to avoid creating new instance we need to create new instance, since the validator gets a qos as c'tor param. 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.. same. 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. same Line 23: return new QosValidator<StorageQos>(qos) { Line 24: Line 25: @Override Line 26: protected QosDao<StorageQos> getQosDao() { 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 it's clear to me, and I think that to you as well (I think that you even explained that to Kobi) but I will rename. 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. this check is used in NetworkQos and don't have time to investigate why. 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. To be on the safe side I always checks for nullity before iterating. extracted getAllQosInDc() to a parameter to avoid double call. 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 ra I will open a bug. 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 it's more trickier than you think. some parameters can be null and some not (the logic is not that simple). you will see it once I will implement it. 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. not following, which one? 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
