Ori Liel has posted comments on this change. Change subject: restapi: implement Quota REST-API ......................................................................
Patch Set 1: (11 comments) https://gerrit.ovirt.org/#/c/39904/1/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendClusterQuotaLimitsResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendClusterQuotaLimitsResource.java: Line 26: quotaCluster 1) Using cluster-ID as the limitation ID implies that no more than 1 cluster-limitation is possible (otherwise yo'd have 2 limitations with the same ID). If this is the case, why implement cluster-limitation as a collection (rather than show the limitation inline in the quota)? And why the for loop here? If it's because we may want to support more than 1 cluster-limitation per quota in the future, then we can't use the cluster-ID as the cluster-limitation ID. 2) You're using existing IDs for new entities. I don't know what the implications could be of having two entities in the system (even if they are of different types) that have the same ID. Use GuidUtils-->generateGuidUsingMd5() to hash these IDs into new IDs Line 58: . I think this isn't the right way to delete an item from a collection while looping over the collection. Better to use iterator.remove() https://gerrit.ovirt.org/#/c/39904/1/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendQuotaLimitResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendQuotaLimitResource.java: Line 8: import org.ovirt.engine.core.compat.Guid; Line 9: Line 10: public abstract class BackendQuotaLimitResource<T extends BaseResource> extends AbstractBackendSubResource<T, Quota> implements Line 11: QuotaLimitResource<T> { Line 12: private static final String[] SUB_COLLECTIONS = {}; No need if it's empty Line 13: private final Guid parentId; Line 14: Line 15: protected BackendQuotaLimitResource(String id, Line 16: Guid parentId, Line 29: return super.map(entity, createQuotaLimit()); Line 30: } Line 31: Line 32: @Override Line 33: protected T doPopulate(T model, Quota entity) { No need to override this, enough that it's overriden in ancestor Line 34: return model; Line 35: } Line 36: Line 37: protected abstract T createQuotaLimit(); https://gerrit.ovirt.org/#/c/39904/1/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendQuotaLimitsResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendQuotaLimitsResource.java: Line 16: Line 17: public abstract class BackendQuotaLimitsResource<M extends BaseResources, N extends BaseResource> extends AbstractBackendCollectionResource<N, Quota> implements QuotaLimitsResource<M, N> { Line 18: Line 19: protected final Guid quotaId; Line 20: private static final String[] SUB_COLLECTIONS = {}; No need for this variable. Line 21: Line 22: protected BackendQuotaLimitsResource(Guid quotaId, Line 23: Class<N> baseResourcesClass) { Line 24: super(baseResourcesClass, Quota.class, SUB_COLLECTIONS); Line 20: private static final String[] SUB_COLLECTIONS = {}; Line 21: Line 22: protected BackendQuotaLimitsResource(Guid quotaId, Line 23: Class<N> baseResourcesClass) { Line 24: super(baseResourcesClass, Quota.class, SUB_COLLECTIONS); The constructor will work without SUB_COLLECTIONS Line 25: this.quotaId = quotaId; Line 26: } Line 27: Line 28: protected abstract void updateEntityForRemove(Quota entity, Guid id); Line 50: add Consider improving the API of the engine. It's a burden on clients to have to fetch the entire Quota, add the limitation and then send the whole quota back. Much better IMO would be better to have a AddLimitationCommand. It's also better in terms of performance: one less query necessary, and also less data over the wire. https://gerrit.ovirt.org/#/c/39904/1/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendQuotasResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendQuotasResource.java: Line 43: GuidUtils No need for GuidUtils, there's a asGuid(String guid) method in ancestor class. Line 48: quota add call to validateParameters() method and list the mandatory parameters. https://gerrit.ovirt.org/#/c/39904/1/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageQuotaLimitsResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageQuotaLimitsResource.java: Line 22: if (quota.getGlobalQuotaStorage() != null) { Line 23: addLimit(quota.getGlobalQuotaStorage(), quotaId.toString(), limits, quota); Line 24: } else if (quota.getQuotaStorages() != null) { Line 25: for (QuotaStorage quotaStorage : quota.getQuotaStorages()) { Line 26: addLimit(quotaStorage, quotaStorage.getStorageId().toString(), limits, quota); I think there can be more than one storage-limit. Giving the storage limit the same ID as the QuotaStorage may result in 2 limits having the same ID. See more elaborate comment in BackendClusterQuotaLimitsResource.java Line 27: } Line 28: } Line 29: return limits; Line 30: } https://gerrit.ovirt.org/#/c/39904/1/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/QuotaMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/QuotaMapper.java: Line 85: getQuotaStorages I think it's unnecessary to map storage limitations, because they are displayed in a different context (not inside the quota) I would write a mapping method: QuotaStorage-->StorageQuotaLimit, and invoke it in .../limitations context. -- To view, visit https://gerrit.ovirt.org/39904 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifaead35839c2c5f89f1551e67e58a58aa253cab5 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Ori Liel <[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
