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

Reply via email to