Arik Hadas has posted comments on this change. Change subject: backend: Add HostDev passthrough support #1 ......................................................................
Patch Set 12: (12 comments) http://gerrit.ovirt.org/#/c/35892/12//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2014-12-03 17:09:16 +0100 Line 4: Commit: Martin Betak <[email protected]> Line 5: CommitDate: 2015-01-15 16:25:53 +0100 Line 6: Line 7: backend: Add HostDev passthrough support #1 what does the '#1' stands for? Line 8: Line 9: Preliminary steps for providing the ability to pass through Line 10: selected devices from Hosts to VMs. Line 11: http://gerrit.ovirt.org/#/c/35892/12/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandsFactory.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandsFactory.java: Line 135: (QueriesCommandBase<?>) findCommandConstructor(type, parameters.getClass(), EngineContext.class).newInstance(parameters, Line 136: engineContext); Line 137: Line 138: } Line 139: Injector.injectMembers(result); how about replacing it with "return Injector.injectMembers(result);" in order to be consistent with createCommand? Line 140: return result; Line 141: } catch (Exception e) { Line 142: log.error("Command Factory: Failed to create command '{}' using reflection: {}", type, e.getMessage()); Line 143: log.error("Exception", e); http://gerrit.ovirt.org/#/c/35892/12/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/hostdev/RefreshHostDevicesCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/hostdev/RefreshHostDevicesCommand.java: Line 18: import java.util.HashMap; Line 19: import java.util.List; Line 20: import java.util.Map; Line 21: Line 22: @NonTransactiveCommandAttribute(forceCompensation = true) why is compensation needed? Line 23: public class RefreshHostDevicesCommand<T extends VdsActionParameters> extends RefreshHostInfoCommandBase<T> { Line 24: Line 25: @Inject Line 26: private ResourceManager resourceManager; Line 46: Line 47: for (Map.Entry<String, HostDevice> entry : fetchedMap.entrySet()) { Line 48: HostDevice device = entry.getValue(); Line 49: if (oldMap.containsKey(entry.getKey())) { Line 50: if (!oldMap.get(entry.getKey()).equals(device)) { shouldn't it be !oldMap.get(entry.getValue()).equals(device)? Line 51: changedDevices.add(device); Line 52: } Line 53: } else { Line 54: newDevices.add(device); Line 52: } Line 53: } else { Line 54: newDevices.add(device); Line 55: } Line 56: } how about extracting all this piece of code to method like 'deviceIntersection' (see ImagesHandler#imagesIntersection). yeah, it would mean that the code will be a bit less efficient (another loop over the lists) but since we don't expect the devices list to be that long, I prefer readability over efficient in this particular case Line 57: Line 58: final List<HostDevice> removedDevices = new ArrayList<>(); Line 59: for (Map.Entry<String, HostDevice> entry : oldMap.entrySet()) { Line 60: if (!fetchedMap.containsKey(entry.getKey())) { http://gerrit.ovirt.org/#/c/35892/12/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/HostDevice.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/HostDevice.java: Line 4: import org.ovirt.engine.core.compat.Guid; Line 5: Line 6: public class HostDevice implements BusinessEntity<HostDeviceId> { Line 7: Line 8: public static final Integer NO_IOMMU_GROUP = null; please add a comment that explains why this field is 'public static' and what is it used for Line 9: Line 10: private Guid hostId; Line 11: private String deviceName; Line 12: private String parentDeviceName; http://gerrit.ovirt.org/#/c/35892/12/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/HostDeviceParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/HostDeviceParameters.java: Line 1: package org.ovirt.engine.core.common.queries; Line 2: Line 3: import org.ovirt.engine.core.compat.Guid; Line 4: Line 5: public class HostDeviceParameters extends VdcQueryParametersBase { how about rename it to GetHostDeviceByHostIdAndDeviceNameParameters if it is used only by GetHostDeviceByHostIdAndDeviceNameQuery? Line 6: Line 7: private Guid hostId; Line 8: private String deviceName; Line 9: http://gerrit.ovirt.org/#/c/35892/12/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/MassOperationsDao.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/MassOperationsDao.java: Line 41: * @param ids Line 42: */ Line 43: void removeAll(Collection<ID> ids); Line 44: Line 45: // TODO: batch remove all should probably use only IDs (and id parameter source) as its non-batch counterpart please put it inside the the method's documentation comment Line 46: /** Line 47: * Calls a remove stored procedure multiple times in a batch Line 48: * Line 49: * @param entities Line 62: * Calls an insert stored procedure multiple times in a batch Line 63: * Line 64: * @param entities Line 65: */ Line 66: void saveAll(Collection<T> entities); the comment is wrong, should be for the method below Line 67: Line 68: /** Line 69: * Saves the given entities using a more efficient method to save all of them at once, rather than each at a time. Line 70: * The procedure name to be used is "InsertEntityName" where EntityName stands for the name of the entity http://gerrit.ovirt.org/#/c/35892/12/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsBrokerObjectsBuilder.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsBrokerObjectsBuilder.java: Line 1855: List<HostDevice> devices = new ArrayList<>(); Line 1856: Line 1857: for (Entry<String, Map<String, Object>> entry : deviceList.entrySet()) { Line 1858: Line 1859: Map<String, Object> params = (Map<String, Object>) entry.getValue().get(VdsProperties.PARAMS); can't we define deviceList as: Map<String, Map<String, Map<String, String>>> and remove few casts? Line 1860: String deviceName = entry.getKey(); Line 1861: Line 1862: HostDevice device = new HostDevice(); Line 1863: device.setDeviceName(entry.getKey()); http://gerrit.ovirt.org/#/c/35892/12/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsProperties.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsProperties.java: Line 385: public static final String QOS_PEAK = "peak"; Line 386: public static final String QOS_BURST = "burst"; Line 387: Line 388: // host devices Line 389: public static final String ROOT_DEVICE = "computer"; how about rename to ROOT_HOST_DEVICE or something like that to make its name more meaningful? Line 390: public static final String DEVICE_LIST = "deviceList"; Line 391: public static final String PARAMS = "params"; Line 392: public static final String CAPABILITY = "capability"; Line 393: public static final String IOMMU_GROUP = "iommu_group"; http://gerrit.ovirt.org/#/c/35892/12/packaging/dbscripts/host_device_sp.sql File packaging/dbscripts/host_device_sp.sql: Line 112: WHERE host_id = v_host_id AND device_name = v_device_name; Line 113: END; $procedure$ Line 114: LANGUAGE plpgsql; Line 115: Line 116: CREATE OR REPLACE FUNCTION GetAllFromDevices() s\GetAllFromDevices\GetAllFromHostDevices Line 117: RETURNS SETOF host_device STABLE Line 118: AS $procedure$ Line 119: BEGIN Line 120: RETURN QUERY -- To view, visit http://gerrit.ovirt.org/35892 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia5575c0db797d7d04339c4b309bb4325e853ffed Gerrit-PatchSet: 12 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Betak <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Martin Betak <[email protected]> Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: Shahar Havivi <[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
