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

Reply via email to