Omer Frenkel 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? yes! 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? Done 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? right now yes, but unplug is "right around the corner" there is already a patch in gerrit that enables it for 3.6, since we will not make it on time we separated plug and unplug but its the same bll command. we also not sure when unplug will be fully available 'down the stack' you can see unplug patch at https://gerrit.ovirt.org/#/c/41294/1 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 ca Done (good catch there was no check like this) 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? Done 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 ins Done 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? Done 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 le Done 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? Done -- 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
