Arik Hadas has posted comments on this change.
Change subject: core: Introduce sla package
......................................................................
Patch Set 1: (14 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MemoryVdsComparer.java
Line 5:
Line 6: /**
Line 7: * This comparer chose Vds with more memory available as best
Line 8: */
Line 9: public class MemoryVdsComparer extends VdsComparer {
I think this class should move to sla package as well
Line 10: @Override
Line 11: public boolean IsBetter(VDS x, VDS y, VM vm) {
Line 12: return ((x.getPhysicalMemMb() - x.getMemCommited()) <
(y.getPhysicalMemMb() - y.getMemCommited()));
Line 13:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommandBase.java
Line 48: */
Line 49: public abstract class RunVmCommandBase<T extends
VmOperationParameterBase> extends VmCommand<T> implements
Line 50: IVdsAsyncCommand, RunVmDelayer {
Line 51:
Line 52: public static Log log = LogFactory.getLog(RunVmCommandBase.class);
why changing the visibility?
Line 53: protected static final HashMap<Guid, Integer>
_vds_pending_vm_count = new HashMap<Guid, Integer>();
Line 54: private VdsSelector privateVdsSelector;
Line 55: protected boolean _isRerun = false;
Line 56: protected VDS _destinationVds;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/sla/a.java
Line 1: package org.ovirt.engine.core.bll.sla;
Line 2:
Line 3: public class a {
was it pushed by mistake?
Line 4:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/sla/MigrationHandler.java
Line 4:
Line 5: import org.ovirt.engine.core.common.utils.Pair;
Line 6: import org.ovirt.engine.core.compat.Guid;
Line 7:
Line 8: public abstract class MigrationHandler {
since this class contains only a method signature, can we change it to
interface?
Line 9: /**
Line 10: * this method holds a list of pairs VM id and Host id. each VM
should be migrated to the specified Host
Line 11: * @param list
Line 12: */
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/sla/NoneComparer.java
Line 1: package org.ovirt.engine.core.bll.sla;
Line 2:
Line 3:
Line 4: public class NoneComparer extends EvenlyDistributeComparer {
can we change it to be final class? no one should ever inherit it..
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/sla/SlaValidator.java
Line 5: import org.ovirt.engine.core.utils.log.Log;
Line 6: import org.ovirt.engine.core.utils.log.LogFactory;
Line 7:
Line 8: public class SlaValidator {
Line 9: public static Log log = LogFactory.getLog(SlaValidator.class);
why public?
Line 10:
Line 11: private static final SlaValidator instance = new SlaValidator();
Line 12:
Line 13: public static SlaValidator getInstance() {
Line 13: public static SlaValidator getInstance() {
Line 14: return instance;
Line 15: }
Line 16:
Line 17: public boolean hasMemoryToRunVM(VDS curVds, VM vm) {
I know you only moved this file from RunVmCommandBase, but I really don't like
this programming style of having "structs" (e.g VM, VDS..) and to send them to
static methods - it reminds me the time I developed in C.. don't you think this
method and hasCpuToRunVM should move to VDS class?
Line 18: boolean retVal = false;
Line 19: if (curVds.getMemCommited() != null &&
curVds.getPhysicalMemMb() != null && curVds.getReservedMem() != null) {
Line 20: double vdsCurrentMem =
Line 21: curVds.getMemCommited() +
curVds.getPendingVmemSize() + curVds.getGuestOverhead() + curVds
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/sla/VdsComparer.java
Line 6:
Line 7: /**
Line 8: * Base class for comparing between Hosts
Line 9: */
Line 10: public abstract class VdsComparer {
please consider changing this class to be interface and instead of the factory
method, let each value in VdsSelectionAlgorithm enum to have a method that
returns the appropriate comparer (otherwise, rename CreateComparer to
createComparer)
Line 11: /**
Line 12: * Factory method, creates necessary comparer
Line 13: *
Line 14: * @return
Line 28: * Base abstract function for finish best Vds treatment
Line 29: *
Line 30: * @param x
Line 31: */
Line 32: public abstract void BestVdsProcedure(VDS x);
change to start with lowercase
Line 33:
Line 34: /**
Line 35: * Base abstract function to compare between two VDSs
Line 36: *
Line 38: * @param y
Line 39: * @param vm
Line 40: * @return
Line 41: */
Line 42: public abstract boolean IsBetter(VDS x, VDS y, VM vm);
change to start with lowercase
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/sla/VdsLoadBalancer.java
Line 53: log.infoFormat("VdsLoadBalancer: high util: {0}, low
util: {1}, duration: {2}, threashold: {3}",
Line 54: group.gethigh_utilization(),
group.getlow_utilization(),
Line 55: group.getcpu_over_commit_duration_minutes(),
Line 56: Config.<Integer>
GetValue(ConfigValues.UtilizationThresholdInPercent));
Line 57:
migrationHandler.migrateVMs(loadBalancingAlgorithm.LoadBalance());
null check is missing
Line 58: } else {
Line 59: log.debugFormat("VdsLoadBalancer: Cluster {0} skipped
because no selection algorithm selected.",
Line 60: group.getname());
Line 61: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/sla/VdsLoadBalancingAlgorithm.java
Line 105: // return new VmCountVdsLoadBalancingAlgorithm();
Line 106: return new VdsCpuVdsLoadBalancingAlgorithm(group);
Line 107: }
Line 108:
Line 109: public List<Pair<Guid, Guid>> LoadBalance() {
it would be nice to have javadoc that explains what is the expected list that
is returned from this method (I understand, but to have it documented)
Line 110: List<Pair<Guid, Guid>> migrationList = new
ArrayList<Pair<Guid, Guid>>();
Line 111:
setAllRelevantVdss(DbFacade.getInstance().getVdsDao().getAllForVdsGroupWithoutMigrating(getVdsGroup().getId()));
Line 112: log.infoFormat("VdsLoadBalancer: number of relevant vdss (no
migration, no pending): {0}.",
Line 113: getAllRelevantVdss().size());
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/sla/VdsSelector.java
Line 181: StringBuilder sb = new StringBuilder();
Line 182: for (VDS curVds : vdss) {
Line 183: noVDSs = false;
Line 184:
Line 185: VdcBllMessages result = validateHostIsReadyToRun(curVds,
sb, isMigrate);
this comment is related to all the changes below - why did you make those
changes? the standard way in our system is that validate* methods return
ValidationResult
Line 186: if (result == null) {
Line 187: return true;
Line 188: } else {
Line 189: messageToReturn = result;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsComparer.java
Line 5
Line 6
Line 7
Line 8
Line 9
why remove+add and not rename (in order to preserve the file history) ?
--
To view, visit http://gerrit.ovirt.org/14580
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icac8f7bc8a696455134bb90ffc17afd420e18dd3
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Arik Hadas <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Laszlo Hornyak <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Mike Kolesnik <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Roy Golan <[email protected]>
Gerrit-Reviewer: ofri masad <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
