Martin Sivák has posted comments on this change. Change subject: core: Push ioTune QoS info when hotplugging disk ......................................................................
Patch Set 3: (4 comments) http://gerrit.ovirt.org/#/c/33907/3/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HotPlugDiskVDSCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HotPlugDiskVDSCommand.java: Line 69: drive.put(VdsProperties.PropagateErrors, disk.getPropagateErrors().toString().toLowerCase()); Line 70: Line 71: // maps to avoid fetching qos object for same disk profile id Line 72: Map<Guid, Guid> diskProfileStorageQosMap = new HashMap<>(); Line 73: Map<Guid, Map<String, Long>> storageQosIoTuneMap = new HashMap<>(); > simply call the buildIoTune with new HashMap() <> instead of variables or c What difference will that make? The bytecode will end up the same and it will be less readable. We could avoid creating the cache maps and instead pass null and add couple of ifs to the fetching logic. It will save couple of bytes of memory at the cost of more cluttered code. Line 74: Map<String, Long> ioTune = Line 75: VmInfoBuilder.buildIoTune(diskImage, diskProfileStorageQosMap, storageQosIoTuneMap); Line 76: if (ioTune != null) { Line 77: if (vmDevice.getSpecParams() == null) { Line 72: Map<Guid, Guid> diskProfileStorageQosMap = new HashMap<>(); Line 73: Map<Guid, Map<String, Long>> storageQosIoTuneMap = new HashMap<>(); Line 74: Map<String, Long> ioTune = Line 75: VmInfoBuilder.buildIoTune(diskImage, diskProfileStorageQosMap, storageQosIoTuneMap); Line 76: if (ioTune != null) { > yes this should go to some helper. Guys are you aware of the fact that specParams are not ioTune specific? I can move that to another static method in VmInfoBuilder easy enough though. Line 77: if (vmDevice.getSpecParams() == null) { Line 78: vmDevice.setSpecParams(new HashMap<String, Object>()); Line 79: } Line 80: vmDevice.getSpecParams().put(VdsProperties.Iotune, ioTune); http://gerrit.ovirt.org/#/c/33907/3/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VmInfoBuilder.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VmInfoBuilder.java: Line 361: Line 362: ArchStrategyFactory.getStrategy(vm.getClusterArch()).run(new CreateAdditionalControllers(devices)); Line 363: } Line 364: Line 365: static Map<String, Long> buildIoTune(DiskImage diskImage, > it's custom to declare access modifier. No modifier has a special meaning. Package private and not accessible from subclass. https://docs.oracle.com/javase/tutorial/java/javaOO/accesscontrol.html Line 366: Map<Guid, Guid> diskProfileStorageQosMap, Line 367: Map<Guid, Map<String, Long>> storageQosIoTuneMap) { Line 368: Guid diskProfileId = diskImage.getDiskProfileId(); Line 369: if (diskProfileId == null) { Line 387: } Line 388: return null; Line 389: } Line 390: Line 391: static private Map<String, Long> buildIoTuneMap(StorageQos storageQos) { > s/private static/static private/ That is exactly what I see in the patch.. or do you mean to use private static? Btw who cares about the keyword order? Line 392: // build map Line 393: Map<String, Long> ioTuneMap = new HashMap<>(); Line 394: if (storageQos.getMaxThroughput() != null) { Line 395: // Convert MiB/s to B/s vdsm is expecting -- To view, visit http://gerrit.ovirt.org/33907 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic4bb85cd307089088be77cff28adbc783ebcaedd Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Sivák <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Martin Sivák <[email protected]> Gerrit-Reviewer: Roy Golan <[email protected]> Gerrit-Reviewer: Tomer Saban <[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
