Juan Hernandez has posted comments on this change. Change subject: restapi: added support for io_threads ......................................................................
Patch Set 4: (3 comments) https://gerrit.ovirt.org/#/c/40094/4/backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd File backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd: Line 3259: <xs:sequence> Line 3260: <xs:element name="type" type="xs:string" minOccurs="0"/> Line 3261: <xs:element ref="status" minOccurs="0" maxOccurs="1"/> Line 3262: <xs:element name="memory" type="xs:long" minOccurs="0"/> Line 3263: <xs:element name="num_of_iothreads" type="xs:int" minOccurs="0" maxOccurs="1"/> Currently the RESTAPI doesn't use prefixes like "num_of_" or "amount_of_", see for example the "memory" element above, it isn't "amount_of_memory" but just "memory". Consider renaming to just "io_threads". Are there other related parameters associated to IO (current or future)? If so it may make sense to have a "io" element with nested elements: <io> <threads>...</threads> <some_other_future_tunnable>...</some_other_future_tunnable> </io> Line 3264: <xs:element name="cpu" type="CPU" minOccurs="0"/> Line 3265: <xs:element name="cpu_shares" type="xs:int" minOccurs="0" maxOccurs="1"/> Line 3266: <xs:element name="bios" type="Bios" minOccurs="0" maxOccurs="1" /> Line 3267: <xs:element name="os" type="OperatingSystem" minOccurs="0"/> https://gerrit.ovirt.org/#/c/40094/4/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmBaseMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmBaseMapper.java: Line 182: model.setMemory((long) entity.getMemSizeMb() * BYTES_PER_MB); Line 183: Line 184: if (entity.getNumOfIoThreads() != 0) { Line 185: model.setNumOfIothreads(entity.getNumOfIoThreads()); Line 186: } It would be better if the backend returned "null" when there is no value. Line 187: Line 188: if (entity.getVdsGroupId() != null) { Line 189: Cluster cluster = new Cluster(); Line 190: cluster.setId(entity.getVdsGroupId().toString()); https://gerrit.ovirt.org/#/c/40094/4/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmMapper.java: Line 168: staticVm.setMigrationDowntime(entity.getMigrationDowntime()); Line 169: staticVm.setMinAllocatedMem(entity.getMinAllocatedMem()); Line 170: if (FeatureSupported.isIoThreadsSupported(clusterVersion)) { Line 171: staticVm.setNumOfIoThreads(entity.getNumOfIoThreads()); Line 172: } Shouldn't the backend return "null" for this if the cluster doesn't support IO threads? That would be better from the RESTAPI point of view, as that way the RESTAPI doesn't need to know this business logic rule. Line 173: return staticVm; Line 174: } Line 175: Line 176: @Mapping(from = VM.class, to = VmStatic.class) -- To view, visit https://gerrit.ovirt.org/40094 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3e6f9b76d424027ef21e17cc3b3c9ffd7023b52d Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Tomas Jelinek <tjeli...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
