Ori Liel has posted comments on this change. Change subject: restapi: Introducing Scheduling Policy API ......................................................................
Patch Set 2: (25 comments) 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 <gchap...@redhat.com> General comments for this patch - 1) Use of generics in the interfaces, as you did, might cause RsdlBuilder to break (because of its limitations, not because your use of generics is wrong). Please verify that RSDL generation works, otherwise we'd be breaking: .../api?rsdl 2) There are no tests at all for all of this new functionality, except for mapping, which you did test. Please add some unit-tests to cover the new functionality. 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'? 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 ok, I think you don't need R. 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 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 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 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 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? 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 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: .../api/schedulingpolicies/xxx/balances the ID give to the constructor is the scheduling-policy ID. This is not the same, since the a scheduling-policy may be used by two or more clusters. 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 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, rather than fetching the object in it's context like everywhere else in the API; i.e, in the get() method of BackendBalanceResource? 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 scheculingPolicyUnit then you wouldn' tneed this method 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, rather than fetching the object in it's context like everywhere else in the API; i.e, in the get() method of BackendFilterResource? 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 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 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 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 to any single cluster 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 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: a) 'properties' will exist inside the scheduling policy itself b) anyway all mapping methods should have only from, to parameters; these methods are scanned by MappingHelper by reflection and it wouldn't know how to deal with a mapping method which has 3 parameters. 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 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 API for any north-bound enum in the engine. The reason is to decouple the enigine from the api. As written here, if tomorrow a developer will change the name of some element in the engine enum: PolicyUntiType.java, API will break. 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. 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 in this line you are allowing filter to be null and simply initializing a new Filter object. BTW - pls add documentation that these mappers shouldn't receive 'null' for the second parameter. 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 and you need to update it? 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 <gchap...@redhat.com> Gerrit-Reviewer: Artyom Lukianov <aluki...@redhat.com> Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Martin Sivák <msi...@redhat.com> Gerrit-Reviewer: Ori Liel <ol...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
