Moti Asayag has posted comments on this change.

Change subject: core: Add CRUD commands and queries for QoS objects (storage)
......................................................................


Patch Set 6: Code-Review-1

(6 comments)

audit-log ids should be unique across audit-log type enum.

http://gerrit.ovirt.org/#/c/27096/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/qos/AddStorageQosCommand.java
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/qos/AddStorageQosCommand.java:

Line 18:     }
Line 19: 
Line 20:     @Override
Line 21:     protected QosValidator<StorageQos> getQosValidator(StorageQos qos) 
{
Line 22:         return new QosValidator<StorageQos>(qos) {
how do you plan to add unit-tests for an anonymous class ?
Line 23: 
Line 24:             @Override
Line 25:             protected QosDao<StorageQos> getQosDao() {
Line 26:                 return AddStorageQosCommand.this.getQosDao();


Line 22:         return new QosValidator<StorageQos>(qos) {
Line 23: 
Line 24:             @Override
Line 25:             protected QosDao<StorageQos> getQosDao() {
Line 26:                 return AddStorageQosCommand.this.getQosDao();
why prefix "AddStorageQosCommand.this." required ?
Line 27:             }
Line 28: 
Line 29:             @Override
Line 30:             public ValidationResult allValuesPresent() {


http://gerrit.ovirt.org/#/c/27096/6/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 8: import org.ovirt.engine.core.common.errors.VdcBllMessages;
Line 9: import org.ovirt.engine.core.dao.qos.QosDao;
Line 10: 
Line 11: public abstract class QosValidator<T extends BaseQos> {
Line 12: 
please add unit-tests.
Line 13:     private final T qos;
Line 14:     private T oldQos;
Line 15:     private List<T> allQos;
Line 16: 


Line 68: 
Line 69:     /**
Line 70:      * Verify that a QoS entity's name hasn't changed. This assumes 
that QoS entity has been verified to exist.
Line 71:      */
Line 72:     public ValidationResult nameNotChangedOrNotTaken() {
couldn't found usages for this method.
Line 73:         if (!getOldQos().getName().equals(qos.getName())) {
Line 74:             return nameNotTakenInDc();
Line 75:         } else {
Line 76:             return ValidationResult.VALID;


http://gerrit.ovirt.org/#/c/27096/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java
File 
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java:

Line 957:     USER_FAILED_TO_REMOVE_NETWORK_QOS(10103, AuditLogSeverity.ERROR),
Line 958:     USER_UPDATED_NETWORK_QOS(10104),
Line 959:     USER_FAILED_TO_UPDATE_NETWORK_QOS(10105, AuditLogSeverity.ERROR),
Line 960: 
Line 961:     // Qos
the ids of the audit-log types identifies the entries type in the event log 
table.

Please maintain their uniqueness.
Line 962:     USER_ADDED_QOS(10100),
Line 963:     USER_FAILED_TO_ADD_QOS(10101, AuditLogSeverity.ERROR),
Line 964:     USER_REMOVED_QOS(10102),
Line 965:     USER_FAILED_TO_REMOVE_QOS(10103, AuditLogSeverity.ERROR),


http://gerrit.ovirt.org/#/c/27096/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/VdcQueryType.java
File 
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/VdcQueryType.java:

Line 315:     GetAllProviders,
Line 316:     GetAllNetworksForProvider,
Line 317: 
Line 318:     //Network QoS
Line 319:     GetAllNetworkQosByStoragePoolId,
please add a space line
Line 320:     // QoS
Line 321:     GetQosById,
Line 322:     GetAllQosByStoragePoolIdAndType,
Line 323: 


-- 
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: 6
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

Reply via email to