Arik Hadas has posted comments on this change. Change subject: core: Memory Hotplug ......................................................................
Patch Set 5: (9 comments) https://gerrit.ovirt.org/#/c/35081/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HotSetAmountOfMemoryCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HotSetAmountOfMemoryCommand.java: Line 36: public static final String LOGABLE_FIELD_ERROR_MESSAGE = "ErrorMessage"; Line 37: public static final String DEVICE_SIZE_FIELD_KEY = "size"; Line 38: public static final String DEVICE_NODE_FIELD_KEY = "node"; Line 39: Line 40: int memoryToConsume; private? Line 41: Line 42: public HotSetAmountOfMemoryCommand(T parameters) { Line 43: this(parameters, null); Line 44: } Line 46: public HotSetAmountOfMemoryCommand(T parameters, CommandContext commandContext) { Line 47: super(parameters, commandContext); Line 48: memoryToConsume = getParameters().getVm().getMemSizeMb() - getVm().getMemSizeMb(); Line 49: if (getParameters().getNumaNode() == null) { Line 50: getParameters().setNumaNode(0); so why not to use int instead of Integer in the parameters? Line 51: } Line 52: Line 53: } Line 54: Line 86: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_MEMORY_MUST_BE_MULTIPLICATION, Line 87: "$multiplicationSize " + Config.getValue(ConfigValues.HotPlugMemoryMultiplicationSizeMb).toString()); Line 88: } Line 89: } else if (!FeatureSupported.hotUnplugMemory(getVm().getVdsGroupCompatibilityVersion(), getVm().getClusterArch())) { Line 90: return failCanDoAction(VdcBllMessages.HOT_UNPLUG_MEMORY_IS_NOT_SUPPORTED); shouldn't the unplug be blocked for all compatibility version? Line 91: } Line 92: Line 93: return true; Line 94: } Line 153: QuotaConsumptionParameter.QuotaAction.RELEASE, Line 154: getVm().getVdsGroupId(), Line 155: 0, Line 156: Math.abs(memoryToConsume))); Line 157: } how about to simplify it to (assuming memoryToConsume==0 is block by the caller or by CDA): list.add(new QuotaVdsGroupConsumptionParameter(getVm().getQuotaId(), null, X, getVm().getVdsGroupId(), 0, Math.abs(memoryToConsume))); X: memoryToConsume > 0? CONSUME : RELEASE Line 158: return list; Line 159: } Line 160: Line 161: @Override Line 160: Line 161: @Override Line 162: public AuditLogType getAuditLogTypeValue() { Line 163: addCustomValue(LOGABLE_FIELD_NEW_MEMORY, String.valueOf(getParameters().getVm().getMemSizeMb())); Line 164: addCustomValue(LOGABLE_FIELD_PREVIOUS_MEMORY, String.valueOf(getVm().getMemSizeMb())); can these 2 lines move to the case where getSucceeded()==true below? Line 165: Line 166: if (getSucceeded()) { Line 167: return AuditLogType.HOT_SET_MEMORY; Line 168: } else { https://gerrit.ovirt.org/#/c/35081/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java: Line 99: private boolean quotaSanityOnly = false; Line 100: private VmStatic newVmStatic; Line 101: private VdcReturnValueBase setNumberOfCpusResult; Line 102: private List<GraphicsDevice> cachedGraphics; Line 103: private VdcReturnValueBase setAmountOfMemoryResult; how about using a local variable and pass it to auditLogHotSetMemCandos instead of adding it as class field? Line 104: Line 105: public UpdateVmCommand(T parameters) { Line 106: this(parameters, null); Line 107: } Line 287: Line 288: private void hotSetMemory(int newAmountOfMemory) { Line 289: int currentMemory = getVm().getMemSizeMb(); Line 290: Line 291: if (getVm().getStatus() == VMStatus.Up) { it is already checked in HotSetAmountOfMemoryCommand, why to check twice? isn't it the same as shutdown a VM that is already down? Line 292: Line 293: HotSetAmountOfMemoryParameters params = Line 294: new HotSetAmountOfMemoryParameters( Line 295: newVmStatic, https://gerrit.ovirt.org/#/c/35081/5/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllErrors.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllErrors.java: Line 54: replicaErr(55), Line 55: UpdateDevice(56), Line 56: hwInfoErr(57), Line 57: ResizeErr(58), Line 58: hotplugMem(70), I know there is a mix here, but how about using the conventional capital letters and '_'? Line 59: HOT_PLUG_UNPLUG_CPU_ERROR(60), Line 60: V2V_JOB_DOESNT_EXIST(66), Line 61: V2V_NO_SUCH_OVF(67), Line 62: V2V_JOB_NOT_DONE(68), https://gerrit.ovirt.org/#/c/35081/5/frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/frontend/VdsmErrors.properties File frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/frontend/VdsmErrors.properties: Line 398: CINDER_ERROR=An error occurred on Cinder - '${cinderException}' Line 399: V2V_JOB_DOESNT_EXIST=Job Id does not exists Line 400: V2V_NO_SUCH_OVF=OVF file does not exists Line 401: V2V_JOB_NOT_DONE=Job status is not done Line 402: V2V_JOB_ALREADY_EXIST=Job id already exists trailing space? -- To view, visit https://gerrit.ovirt.org/35081 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2c4f262d232429bd50eb62929c35b47dd7f0e68b Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vitor de Lima <vdel...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek <mskri...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@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
