Michael Kublin has posted comments on this change.
Change subject: core: throttle running of VMs (#843058)
......................................................................
Patch Set 5: I would prefer that you didn't submit this
(5 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommandBase.java
Line 49: public abstract class RunVmCommandBase<T extends
VmOperationParameterBase> extends VmCommand<T> implements
Line 50: IVdsAsyncCommand, Throttler {
Line 51:
Line 52: private static Log log = LogFactory.getLog(RunVmCommandBase.class);
Line 53: protected static final HashMap<Guid, Integer>
_vds_pending_vm_count = new HashMap<Guid, Integer>();
First of all you are stepping in to old bug, how that lock will help?
These lock per object. We are running same object like RunVmCommand in
different threads?
Line 54: protected final Lock decreaseLock = new ReentrantLock();
Line 55: protected final Condition decreased = decreaseLock.newCondition();
Line 56: private VdsSelector privateVdsSelector;
Line 57: protected boolean _isRerun = false;
Line 51:
Line 52: private static Log log = LogFactory.getLog(RunVmCommandBase.class);
Line 53: protected static final HashMap<Guid, Integer>
_vds_pending_vm_count = new HashMap<Guid, Integer>();
Line 54: protected final Lock decreaseLock = new ReentrantLock();
Line 55: protected final Condition decreased = decreaseLock.newCondition();
also don't introduce a custom log that are located in some class, we will need
to clean them when we will move to cluster solution.
Line 56: private VdsSelector privateVdsSelector;
Line 57: protected boolean _isRerun = false;
Line 58: protected VDS _destinationVds;
Line 59: private SnapshotsValidator snapshotsValidator=new
SnapshotsValidator();
Line 411: vds.getvds_name(),
vds.getpending_vcpus_count(), getVm().getvm_name());
Line 412: log.debugFormat("DecreasePendingVms::Decreasing
vds {0} pending vmem size, now {1}. Vm: {2}",
Line 413: vds.getvds_name(),
vds.getpending_vmem_size(), getVm().getvm_name());
Line 414: }
Line 415: }
signal is send but dynamic data of vds is not usually updated, why send a
signal? Nothing changed
Line 416: decreased.signal();
Line 417: } finally {
Line 418: decreaseLock.unlock();
Line 419: }
Line 424: * the DecreasePendingVms event.
Line 425: * @see VdsEventListener
Line 426: * @See VdsUpdateRunTimeInfo
Line 427: */
Line 428: @Override
You are using global lock, but throtter is per vds. These is a performance
bottleneck.
Why not use a lock utill?
Line 429: public void throttle(Guid vdsId) {
Line 430: if (log.isDebugEnabled()) {
Line 431: log.debug("try to wait for te engine update the host
memory and cpu stats");
Line 432: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsFreeMemoryChecker.java
Line 6: import org.ovirt.engine.core.dal.dbbroker.DbFacade;
Line 7: import org.ovirt.engine.core.utils.log.LogFactory;
Line 8:
Line 9: public class VdsFreeMemoryChecker {
Line 10:
Each time different throttler with different locks
Line 11: private Throttler throttler;
Line 12:
Line 13: private static Log log =
LogFactory.getLog(VdsFreeMemoryChecker.class);
Line 14:
--
To view, visit http://gerrit.ovirt.org/7204
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I076ede6cba919bc61f7546d7b29ef436eb6d3375
Gerrit-PatchSet: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Roy Golan <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Moti Asayag <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Roy Golan <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
