Michael Pasternak has posted comments on this change.

Change subject: WIP restapi: Add fields for CPU Thread Handling (4/5)
......................................................................


Patch Set 2: I would prefer that you didn't submit this

(4 inline comments)

....................................................
File 
backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd
Line 400:             <jaxb:property generateIsSetMethod="false"/>
Line 401:           </xs:appinfo>
Line 402:         </xs:annotation>
Line 403:     </xs:attribute>
Line 404:     <xs:attribute name="ht_enabled" type="xs:boolean" use="optional">
- this should be a part of "CPU" type and not "CpuTopology"

- this should be an element and not attribute
Line 405:         <xs:annotation>
Line 406:           <xs:appinfo>
Line 407:             <jaxb:property generateIsSetMethod="false"/>
Line 408:           </xs:appinfo>


Line 407:             <jaxb:property generateIsSetMethod="false"/>
Line 408:           </xs:appinfo>
Line 409:         </xs:annotation>
Line 410:     </xs:attribute>
Line 411:     <xs:attribute name="vdsm_count_threads_as_cores" 
type="xs:boolean" use="optional">
- this should be a part of "CPU" type and not "CpuTopology"

- this should be an element and not attribute

- i'd go with threads_as_cores rather than vdsm_count_threads_as_cores
Line 412:         <xs:annotation>
Line 413:           <xs:appinfo>
Line 414:             <jaxb:property generateIsSetMethod="false"/>
Line 415:           </xs:appinfo>


Line 1044:           <xs:element name="supported_versions" 
type="SupportedVersions" minOccurs="0"/>
Line 1045:           <xs:element name="error_handling" type="ErrorHandling" 
minOccurs="0" maxOccurs="1"/>          
Line 1046:           <xs:element name="virt_service" type="xs:boolean" 
minOccurs="0" maxOccurs="1"/>
Line 1047:           <xs:element name="gluster_service" type="xs:boolean" 
minOccurs="0" maxOccurs="1"/>
Line 1048:           <xs:element name="count_threads_as_cores" 
type="xs:boolean" minOccurs="0" maxOccurs="1"/>
host can be attached only to one cluster at the same time, so why do you need 
defining threads_as_cores in two places host & cluster?
Line 1049:           <!-- Also a rel="networks" link -->
Line 1050:         </xs:sequence>
Line 1051:       </xs:extension>
Line 1052:     </xs:complexContent>


....................................................
File 
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/utils/FeaturesHelper.java
Line 176:         feature.getHost().setCpu(new CPU());
Line 177:         feature.getHost().getCpu().setTopology(new CpuTopology());
Line 178:         feature.getHost().getCpu().getTopology().setSockets(4);
Line 179:         feature.getHost().getCpu().getTopology().setHtEnabled(true);
Line 180:         
feature.getHost().getCpu().getTopology().setVdsmCountThreadsAsCores(false);
this two should be added when version >= 3.2 (if you plan this feat for 3.2)
Line 181:         features.getFeature().add(feature);
Line 182:     }
Line 183: 
Line 184:     private void addFeatureHostMemory(Features features) {


--
To view, visit http://gerrit.ovirt.org/10169
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iee8eebee7db2611276f58169aef196053542c7a5
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett <gpadg...@redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com>
Gerrit-Reviewer: Greg Padgett <gpadg...@redhat.com>
Gerrit-Reviewer: Michael Pasternak <mpast...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to