Gilad Chaplik has posted comments on this change. Change subject: restapi: Introducing Scheduling Policy API ......................................................................
Patch Set 2: (25 comments) @Juan, can you please assist in comment #1 (Ori's comment in commit msg) As a result of code review 2 bugs will be opened (targeted to 3.5): * Tests (code change). * Update (PUT) Filter & Weight elements. http://gerrit.ovirt.org/#/c/28093/2//COMMIT_MSG Commit Message: Line 10: Line 11: Change-Id: I6c419f953a533e94d8be17cd9ba915ccd5a617a6 Line 12: Bug-Url: https://bugzilla.redhat.com/1076705 Line 13: Bug-Url: https://bugzilla.redhat.com/1071872 Line 14: Signed-off-by: Gilad Chaplik <[email protected]> > General comments for this patch - 1) what to do/how to resolve that, will prefer not to investigate RsdlBuilder. @Juan, can you please assist? 2) I will add tests in a later patch (open a code change bug for it), sorry don't have time for it before FF; anyway I have some difficulties with how tests are implemented in the project, I will be more than happy if we can have some brainstorming about it, I feel that we can structure the tests better. http://gerrit.ovirt.org/#/c/28093/2/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/PolicyUnitsResource.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/PolicyUnitsResource.java: Line 13: import org.ovirt.engine.api.model.BaseResource; Line 14: import org.ovirt.engine.api.model.BaseResources; Line 15: Line 16: @Produces({ ApiMediaType.APPLICATION_XML, ApiMediaType.APPLICATION_JSON, ApiMediaType.APPLICATION_X_YAML }) Line 17: public interface PolicyUnitsResource<P extends BaseResources, Q extends BaseResource, R extends PolicyUnitResource<Q>> { > what's the use of the generic parameter 'R'? Done Line 18: @GET Line 19: @Formatted Line 20: public P list(); Line 21: Line 28: @Path("{id}") Line 29: public Response remove(@PathParam("id") String id); Line 30: Line 31: @Path("{id}") Line 32: public PolicyUnitResource<Q> getSubResource(@PathParam("id") String id); > I'm guessing you meant to put 'R' here? But since PolicyUnitResource<Q> is Done http://gerrit.ovirt.org/#/c/28093/2/backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd File backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd: Line 680: Line 681: <xs:complexType name="SchedulingPolicies"> Line 682: <xs:complexContent> Line 683: <xs:extension base="BaseResources"> Line 684: <xs:sequence> > whitespace Done Line 685: <xs:element name="scheduling_policy" type="SchedulingPolicy" minOccurs="0" maxOccurs="unbounded"/> Line 686: <!-- deprecated - start --> Line 687: <xs:element name="policy" type="xs:string" minOccurs="0" maxOccurs="unbounded"/> Line 688: <!-- deprecated - end --> Line 1149: <xs:element name="scheduling_policy_unit_types" type="SchedulingPolicyUnitTypes" /> Line 1150: Line 1151: <xs:complexType name="SchedulingPolicyUnitTypes"> Line 1152: <xs:sequence> Line 1153: <xs:element name="scheduling_policy_unit_types" type="xs:string" minOccurs="0" maxOccurs="unbounded"> > scheduling_policy_unit_types-->scheduling_policy_unit_type Done Line 1154: <xs:annotation> Line 1155: <xs:appinfo> Line 1156: <jaxb:property name="SchedulingPolicyUnitTypes" /> Line 1157: </xs:appinfo> Line 1381: <xs:element name="enabled" type="xs:boolean" minOccurs="0" maxOccurs="1"/> Line 1382: <xs:element ref="properties" minOccurs="0" maxOccurs="1"> Line 1383: <xs:annotation> Line 1384: <xs:appinfo> Line 1385: <jaxb:property name="RegexValidationMap"/> > Please think of a better name. Perhaps PropertiesMetaData Done Line 1386: </xs:appinfo> Line 1387: </xs:annotation> Line 1388: </xs:element> Line 1389: </xs:sequence> Line 1465: <xs:complexType name="Weights"> Line 1466: <xs:complexContent> Line 1467: <xs:extension base="BaseResources"> Line 1468: <xs:sequence> Line 1469: <xs:element ref="scheduling_policy" minOccurs="0" maxOccurs="1" /> > remove this backlink Done Line 1470: <xs:element ref="weight" minOccurs="0" maxOccurs="unbounded"> Line 1471: <xs:annotation> Line 1472: <xs:appinfo> Line 1473: <jaxb:property name="Weights" /> Line 1484: <xs:complexType name="Balance"> Line 1485: <xs:complexContent> Line 1486: <xs:extension base="BaseResource"> Line 1487: <xs:sequence> Line 1488: <xs:element ref="scheduling_policy_unit" minOccurs="0" > Where's the back-link to scheduling_policy? Done Line 1489: maxOccurs="1" /> Line 1490: </xs:sequence> Line 1491: </xs:extension> Line 1492: </xs:complexContent> Line 1535: <xs:element ref="cpu" minOccurs="0" maxOccurs="1"/> Line 1536: <xs:element ref="data_center" minOccurs="0" maxOccurs="1"/> Line 1537: <xs:element name="memory_policy" type="MemoryPolicy" minOccurs="0" maxOccurs="1"/> Line 1538: <xs:element name="scheduling_policy" type="SchedulingPolicy" minOccurs="0" maxOccurs="1"/> Line 1539: <xs:element ref="properties" minOccurs="0" maxOccurs="1"> > Place this inside the SchedulingPolicy element Done Line 1540: <xs:annotation> Line 1541: <xs:appinfo> Line 1542: <jaxb:property name="SchedulingPolicyProperties"/> Line 1543: </xs:appinfo> http://gerrit.ovirt.org/#/c/28093/2/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendBalancesResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendBalancesResource.java: Line 12: import org.ovirt.engine.core.compat.Guid; Line 13: Line 14: public class BackendBalancesResource extends BackendPolicyUnitsResource<Balances, Balance, PolicyUnitResource<Balance>> implements BalancesResource { Line 15: Line 16: protected BackendBalancesResource(Guid clusterPolicyId) { > why clusterPolicyId? This object serves: Done Line 17: super(clusterPolicyId, Balance.class); Line 18: } Line 19: Line 20: @Override Line 30: } Line 31: Line 32: @SingleEntityResource Line 33: @Override Line 34: public BalanceResource getSubResource(String id) { > getSubResource --> getBalanceSubResource, for consistency Done. Since this method overrides an interface, created another method to suit consistency. Line 35: return inject(new BackendBalanceResource(id, getPolicyUnit(id))); Line 36: } Line 37: Line 38: @Override Line 31: Line 32: @SingleEntityResource Line 33: @Override Line 34: public BalanceResource getSubResource(String id) { Line 35: return inject(new BackendBalanceResource(id, getPolicyUnit(id))); > Why do you fetch the object here, and provide it to BackendBalanceResource, Done Line 36: } Line 37: Line 38: @Override Line 39: public Balance add(Balance incoming) { Line 65: return map(getClusterPolicy(), balance); Line 66: } Line 67: Line 68: @Override Line 69: protected void updateIncomingId(Balance incoming) { > if filter/weight/balance would inherit a base entity which contains schecul I think it's an overhead for a simple one liner. your call. Line 70: incoming.setId(incoming.getSchedulingPolicyUnit().getId()); Line 71: } Line 72: http://gerrit.ovirt.org/#/c/28093/2/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendFiltersResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendFiltersResource.java: Line 33: Line 34: @SingleEntityResource Line 35: @Override Line 36: public FilterResource getSubResource(String id) { Line 37: return inject(new BackendFilterResource(id, getPolicyUnit(id))); > Why do you fetch the object here, and provide it to BackendFilterResource, Done Line 38: } Line 39: Line 40: @Override Line 41: public Filter add(Filter incoming) { http://gerrit.ovirt.org/#/c/28093/2/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendPolicyUnitsResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendPolicyUnitsResource.java: Line 28: super(baseResourcesClass, ClusterPolicy.class, SUB_COLLECTIONS); Line 29: this.clusterPolicyId = clusterPolicyId; Line 30: } Line 31: Line 32: protected abstract ParametersProvider<N, ClusterPolicy> getParametersProvider(); > rename: getParametersProvider --> getAddParametersProvider Done Line 33: Line 34: protected abstract N getPolicyUnit(String id); Line 35: Line 36: protected abstract void updateEntityForRemove(ClusterPolicy entity, Guid id); Line 47: Line 48: // need to revisit: update should be in a separate hierarchy Line 49: protected N performAdd(N incoming) { Line 50: ClusterPolicy entity = getClusterPolicy(); Line 51: validateUpdate(incoming, map(entity)); > remove this line Done Line 52: performAction(VdcActionType.EditClusterPolicy, getParametersProvider().getParameters(incoming, entity)); Line 53: entity = getClusterPolicy(); Line 54: updateIncomingId(incoming); Line 55: N model = map(entity, incoming); Line 60: protected N doPopulate(N model, ClusterPolicy entity) { Line 61: return model; Line 62: } Line 63: Line 64: protected void validateUpdate(N incoming, N existing) { > remove this code Done Line 65: String reason = localize(Messages.BROKEN_CONSTRAINT_REASON); Line 66: String detail = localize(Messages.BROKEN_CONSTRAINT_DETAIL_TEMPLATE); Line 67: Response error = Line 68: MutabilityAssertor.imposeConstraints(STRICTLY_IMMUTABLE, incoming, existing, reason, detail); http://gerrit.ovirt.org/#/c/28093/2/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendSchedulingPolicyResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendSchedulingPolicyResource.java: Line 26: public SchedulingPolicy get() { Line 27: return performGet(VdcQueryType.GetClusterPolicyById, new IdQueryParameters(guid)); Line 28: } Line 29: Line 30: protected ClusterPolicy getClusterPolicy() { > getClusterPolicy-->getSchedulingPolicy. What you're fetching is not related Done Line 31: return getEntity(new QueryIdResolver<Guid>(VdcQueryType.GetClusterPolicyById, IdQueryParameters.class), false); Line 32: } Line 33: Line 34: @Override http://gerrit.ovirt.org/#/c/28093/2/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendWeightsResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendWeightsResource.java: Line 81: } Line 82: Line 83: @Override Line 84: protected void updateIncomingId(Weight incoming) { Line 85: incoming.setId(incoming.getSchedulingPolicyUnit().getId()); > why is this duplicated in filter/weights/balances? Exactly the same code I think it's an overhead for a simple one liner. your call. Line 86: } Line 87: http://gerrit.ovirt.org/#/c/28093/2/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/ClusterMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/ClusterMapper.java: Line 189: model.getTransparentHugepages().setEnabled(entity.getTransparentHugepages()); Line 190: return model; Line 191: } Line 192: Line 193: public static VDSGroup map(Cluster cluster, SchedulingPolicy model, VDSGroup template) { > remove cluster from here: Done Line 194: VDSGroup entity = template != null ? template : new VDSGroup(); Line 195: if (model.isSetId()) { Line 196: entity.setClusterPolicyId(GuidUtils.asGuid(model.getId())); Line 197: } Line 214: entity.getClusterPolicyProperties().put(CPU_OVER_COMMIT_DURATION_MINUTES, Integer.toString(round)); Line 215: } Line 216: } Line 217: if (cluster.isSetSchedulingPolicyProperties()) { Line 218: entity.setClusterPolicyProperties(CustomPropertiesParser.toMap(cluster.getSchedulingPolicyProperties())); > as said above, remove Done Line 219: } Line 220: return entity; Line 221: } Line 222: http://gerrit.ovirt.org/#/c/28093/2/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/PolicyUnitMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/PolicyUnitMapper.java: Line 15: model.setId(entity.getId().toString()); Line 16: model.setName(entity.getName()); Line 17: model.setDescription(entity.getDescription()); Line 18: Line 19: model.setType(entity.getPolicyUnitType().name().toLowerCase()); > You should have PolicyUnitType enum in the API. We have 'twin' enums in the Done Line 20: model.setEnabled(entity.isEnabled()); Line 21: model.setInternal(entity.isInternal()); Line 22: if (entity.getParameterRegExMap() != null && !entity.getParameterRegExMap().isEmpty()) { Line 23: model.setRegexValidationMap(CustomPropertiesParser.fromMap(entity.getParameterRegExMap())); Line 40: if (model.isSetDescription()) { Line 41: entity.setDescription(model.getDescription()); Line 42: } Line 43: if (model.isSetType()) { Line 44: entity.setPolicyUnitType(PolicyUnitType.valueOf(model.getType().toUpperCase())); > Same, use an enum, and map the enum values. Done Line 45: } Line 46: if (model.isSetEnabled()) { Line 47: entity.setEnabled(model.isEnabled()); Line 48: } http://gerrit.ovirt.org/#/c/28093/2/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/SchedulingPolicyMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/SchedulingPolicyMapper.java: Line 60: Line 61: @Mapping(from = ClusterPolicy.class, to = Filter.class) Line 62: public static Filter map(ClusterPolicy entity, Line 63: Filter template) { Line 64: Filter model = template != null ? template : new Filter(); > This mapper, as it is written, is counting on filter not being null - but i added assert false instead of doc. Done. Line 65: SchedulingPolicyUnit schedulingPolicyUnit = new SchedulingPolicyUnit(); Line 66: schedulingPolicyUnit.setId(model.getId()); Line 67: model.setSchedulingPolicyUnit(schedulingPolicyUnit); Line 68: Integer position = null; Line 111: Guid guid = GuidUtils.asGuid(model.getSchedulingPolicyUnit().getId()); Line 112: if (entity.getFunctions() == null) { Line 113: entity.setFunctions(new ArrayList<Pair<Guid, Integer>>()); Line 114: } Line 115: entity.getFunctions().add(new Pair<>(guid, model.isSetFactor() ? model.getFactor() : 1)); > you are adding a new pair, isn't it possible that this pair already exists I'm not planning to support Update for these objects (same for filter and position). I think it's an overhead and won't be used, anyway I will open a bug for it and follow prioritization. Line 116: } Line 117: Line 118: return entity; Line 119: } -- To view, visit http://gerrit.ovirt.org/28093 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6c419f953a533e94d8be17cd9ba915ccd5a617a6 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Artyom Lukianov <[email protected]> Gerrit-Reviewer: Doron Fediuck <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Martin Sivák <[email protected]> Gerrit-Reviewer: Ori Liel <[email protected]> Gerrit-Reviewer: Ravi Nori <[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
