Liron Ar has posted comments on this change. Change subject: core: Add CRUD commands and queries for QoS objects (storage) ......................................................................
Patch Set 7: (11 comments) 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() { > Can be done in a later patch. matter of coding style (no time...) ok, let's not block on that. 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() { > hope you can let this one slide... ok, let's not block on that. 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. dealing with transactions is irrelevant here, no vdsm calls, only db que 1. it's still relevant, there's no need for distirbuted transaction as you have only one statement that can be executed as a db local transaction. if you can please add it when you rebase. 2. db concurrency isn't good enough, for example, that you you might have concurrent update and delete which will result in audit log of "qos was deleted" followed by "qos was updated" (and there is also the unique name issue that we discussed). you can open a bug for that if you don't want to handle it now..but it's definitely more correct imo. 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 2. my point was missed, i meant that we can cache the qos validator created here and return the same instance on each call to getQosValidator() ..now each time new one will be created. if you want you can change it. 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: > same. see related comment 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) { > same see related comment 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() { > it's clear to me, and I think that to you as well (I think that you even ex in the context of the validator it's not clear imo, and when thinking about it i'd also add that as a javadoc to the dao method because it indeed can be confusing. Line 31: if (allQos == null) { Line 32: allQos = getQosDao().getAllForStoragePoolId(qos.getStoragePoolId()); Line 33: } Line 34: return allQos; 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) { > To be on the safe side I always checks for nullity before iterating. there's no need imo to be on the safe side here, that's the api used all over the engine if it breaks no flow will work so its safe enough, consider that. 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); > I will open a bug. ok 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's more trickier than you think. some parameters can be null and some not ok, so let's agree that the name is misleading :) consider to rename to "validateValues" or something like that. 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 { > not following, which one? the suggestion to seperate the parameters and use the javax validations needed for each one (@NotNull..etc). 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
