Roy Golan has posted comments on this change. Change subject: PendingResourceManager for tracking resources in WaitForLaunch ......................................................................
Patch Set 17: (22 comments) https://gerrit.ovirt.org/#/c/40136/17/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/pending/AbstractPendingResource.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/pending/AbstractPendingResource.java: Line 15: AbstractPendingResource the class is already abstract so just PendingResource is fine Line 16: VDS host; why do we need the whole VDS object? VdsDynamic is a better fit. there's a chance the only the Guid will do Line 61: Objec just put PendingResource https://gerrit.ovirt.org/#/c/40136/17/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/pending/PendingCpuCores.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/pending/PendingCpuCores.java: Line 12: long int is enough https://gerrit.ovirt.org/#/c/40136/17/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/pending/PendingMemory.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/pending/PendingMemory.java: Line 53: pendingHostResources again, the whole VDS object here looks like too much. try narrowing it. https://gerrit.ovirt.org/#/c/40136/17/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/pending/PendingResourceManager.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/pending/PendingResourceManager.java: Line 25: private static final Logger log = LoggerFactory.getLogger(PendingResourceManager.class); Line 26: Line 27: // All internal structures have to be thread-safe for concurrent access Line 28: final Map<Guid, Set<AbstractPendingResource>> resourcesByHost = new ConcurrentHashMap<>(); Line 29: final Map<Guid, Set<AbstractPendingResource>> resourcesByVm = new ConcurrentHashMap<>(); proposing a unified structure with the VM guid as the key. the AbstractPendingResource should have a src/dst Host uid and that way we would access a single record instead of 2. since hosts must know which VM's they deal with, we can deduce from which VM resources we want to clean. that will make the synchronization in line 49 redundant. a pending resource is is comprised of: PendingResource { /* src host */ Guid src; /* dst host */ Guid dst; ... // resource specifics } Line 30: final Map<AbstractPendingResource, AbstractPendingResource> pendingResources = new ConcurrentHashMap<>(); Line 31: Line 32: final ResourceManager resourceManager; Line 33: Line 83: * This is supposed to be used during maintenance flows to clear possible stale data. Line 84: * Line 85: * @param host VDS with valid getId() Line 86: */ Line 87: public void clearHost(VDS host) { again I think a single structure will handle both. Line 88: synchronized (this) { Line 89: if (!resourcesByHost.containsKey(host.getId())) { Line 90: return; Line 91: } Line 160: new ArrayList<>(0); Use Collections.emptyList() instead. its immutable by the way so don't plan on mutation at the client side. Line 164: for (AbstractPendingResource resource: resourcesByHost.get(vds.getId())) { : if (resource.getClass().equals(type)) { : list.add((T)resource); lets use java's type system. getClass() is a code smell and usually mean something is not abstracted. I'll continue with the code review and have more instights on this as I get the full picture Line 178: probably extracted that for line 163, I guess Line 196: if (resourceManager == null) return missing curly braces. //TODO CDI this class Line 200: VDS vds = new VDS(); : vds.setId(hostId); why we need a new VDS instance just to calc the it? https://gerrit.ovirt.org/#/c/40136/17/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/EvenDistributionWeightPolicyUnit.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/EvenDistributionWeightPolicyUnit.java: Line 41: double that's an int. https://gerrit.ovirt.org/#/c/40136/17/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/PinToHostPolicyUnit.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/PinToHostPolicyUnit.java: Line 20: pendingResourceManager) tough needed in few of the units, all the constructors have been changed to use PRM. definitly should be CDI. I'll heplp with a patch https://gerrit.ovirt.org/#/c/40136/17/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/PowerSavingBalancePolicyUnit.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/PowerSavingBalancePolicyUnit.java: Line 148: ollectForHost ok it took me a while cause the static method on the Resource is confusing. why not getPendingResourceManager().getPending(vdsId).isEmpty() we could type the signature for other usages: getPendingResourceManager().getPending(VM) { ... } https://gerrit.ovirt.org/#/c/40136/17/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VDS.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VDS.java: Line 893: /** kudos for the javadoc Line 899: SchedulingManager.getInstance().getPendingResourceManager() : */ replace with plain link reference[1] to the class - one day (soon) RendingResourceManager would be located by CDI and not SchedulingManager.getInstance() [1] {@linkplain PendingResourceManager} Line 911: * assigned to a running VM. Line 912: * Line 913: * This field is a cache, use for reporting only. Line 914: * The authoritative source for current value is the Line 915: * SchedulingManager.getInstance().getPendingResourceManager() same Line 916: */ Line 917: public int getPendingVmemSize() { Line 918: return vdsDynamic.getPendingVmemSize(); Line 919: } Line 1211: float why is it a float? split mega is meaningless https://gerrit.ovirt.org/#/c/40136/17/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VdsDynamic.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VdsDynamic.java: Line 72: * as part of not yet running VM. Line 73: * Line 74: * Do not use for anything critical as scheduling, ask for the Line 75: * exact data using SchedulingManager.getInstance().getPendingResourceManager(). Line 76: */ kudos for the javadc again, and same comment about the reference Line 77: private Integer pendingVcpusCount; Line 78: Line 79: private Integer cpuSockets; Line 80: Line 135: * Cached (best effort) amount of memory scheduled Line 136: * as part of not yet running VM. Line 137: * Line 138: * Do not use for anything critical as scheduling, ask for the Line 139: * exact data using SchedulingManager.getInstance().getPendingResourceManager(). same Line 140: */ Line 141: private Integer pendingVmemSize; Line 142: Line 143: private RpmVersion rpmVersion; https://gerrit.ovirt.org/#/c/40136/17/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsDAODbFacadeImpl.java: Line 375: entity.setHighlyAvailableGlobalMaintenance(rs.getBoolean("ha_global_maintenance")); Line 376: entity.setHighlyAvailableLocalMaintenance(rs.getBoolean("ha_local_maintenance")); Line 377: entity.setKdumpStatus(KdumpStatus.valueOfNumber(rs.getInt("kdump_status"))); Line 378: entity.getSupportedRngSources().addAll(VmRngDevice.csvToSourcesSet(rs.getString("supported_rng_sources"))); Line 379: entity.calculateFreeSchedulingMemoryCache(); line 282 setmemCommited() is called already. Line 380: entity.setBootTime((Long) rs.getObject("boot_time")); Line 381: entity.setSELinuxEnforceMode((Integer) rs.getObject("selinux_enforce_mode")); Line 382: entity.setAutoNumaBalancing(AutoNumaBalanceStatus.forValue(rs.getInt("auto_numa_balancing"))); Line 383: entity.setNumaSupport(rs.getBoolean("is_numa_supported")); -- To view, visit https://gerrit.ovirt.org/40136 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie741b8e244acb64e470af83252d90ec134ba7f8e Gerrit-PatchSet: 17 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Sivák <msi...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Martin Sivák <msi...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Tomer Saban <tsa...@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
