Mike Kolesnik has posted comments on this change. Change subject: core: MacPoolManager-->ScopedMacPoolManager ......................................................................
Patch Set 45: Code-Review-1 (11 comments) Almost there, just a few small fixes http://gerrit.ovirt.org/#/c/26800/45/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java: Line 1110: vmInterfaceManager.add(iface, Line 1111: getCompensationContext(), Line 1112: !getParameters().isImportAsNewEntity(), Line 1113: getVm().getOs(), Line 1114: getVdsGroup().getcompatibility_version() Unnecessary change Line 1115: ); Line 1116: macsAdded.add(iface.getMacAddress()); Line 1117: } Line 1118: http://gerrit.ovirt.org/#/c/26800/45/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommand.java: Line 392: return permissionCheckSubject; Line 393: } Line 394: Line 395: protected void fillMacAddressIfMissing(VmNic iface) { Line 396: final MacPoolManagerStrategy pool = getMacPool(); No need to extract to variable Line 397: if (StringUtils.isEmpty(iface.getMacAddress()) Line 398: && (pool.getAvailableMacsCount() >= 1)) { Line 399: iface.setMacAddress(pool.allocateNewMac()); Line 400: } Line 414: else { Line 415: freeMacs++; Line 416: } Line 417: } Line 418: final MacPoolManagerStrategy macPool = getMacPool(); No need to extract to variable Line 419: if (freeMacs > 0 && !(macPool.getAvailableMacsCount() >= freeMacs)) { Line 420: addCanDoActionMessage(VdcBllMessages.MAC_POOL_NOT_ENOUGH_MAC_ADDRESSES); Line 421: return false; Line 422: } http://gerrit.ovirt.org/#/c/26800/45/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/VmInterfaceManager.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/VmInterfaceManager.java: Line 259: logable.addCustomValue("IfaceName", iface.getName()); Line 260: return logable; Line 261: } Line 262: Line 263: public VmInterfaceManager setMacPool(MacPoolManagerStrategy macPool) { I don't think this makes sense to have as setter, as you always call it after constructing a new instance of this class, so I'd put this in a constructor that accepts this field. Line 264: this.macPool = macPool; Line 265: return this; Line 266: } http://gerrit.ovirt.org/#/c/26800/45/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/macpoolmanager/MacPoolManagerRanges.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/macpoolmanager/MacPoolManagerRanges.java: Line 71: } Line 72: Line 73: /** Line 74: * template method to allow altering initialization process. Line 75: */ This can be deleted Line 76: protected void onInit() { Line 77: } Line 78: Line 79: private void logWhenMacPoolIsEmpty() { http://gerrit.ovirt.org/#/c/26800/45/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/macpoolmanager/MacPoolPerDcSingleton.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/macpoolmanager/MacPoolPerDcSingleton.java: Line 1: package org.ovirt.engine.core.bll.network.macpoolmanager; Line 2: Line 3: public class MacPoolPerDcSingleton { Line 4: //package local & non final for testing Comment can be deleted Line 5: private static MacPoolPerDc INSTANCE = new MacPoolPerDc(); Line 6: Line 7: public static MacPoolPerDc getInstance() { Line 8: return INSTANCE; http://gerrit.ovirt.org/#/c/26800/45/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/vm/AddVmInterfaceCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/vm/AddVmInterfaceCommand.java: Line 32: } Line 33: Line 34: @Override Line 35: protected void executeVmCommand() { Line 36: final VmStatic VM = getVmStaticDAO().get(getParameters().getVmId()); Not necessary to extract to variable Line 37: addCustomValue("InterfaceType", Line 38: (VmInterfaceType.forValue(getInterface().getType()).getDescription()).toString()); Line 39: this.setVmName(VM.getName()); Line 40: Line 42: boolean macAddedToPool = false; Line 43: Line 44: try { Line 45: if (StringUtils.isEmpty(getMacAddress())) { Line 46: No need for this empty line Line 47: getInterface().setMacAddress(getMacPool().allocateNewMac()); Line 48: macAddedToPool = true; Line 49: } else { Line 50: macAddedToPool = addMacToPool(getMacAddress()); http://gerrit.ovirt.org/#/c/26800/45/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/ImportVmTemplateCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/ImportVmTemplateCommandTest.java: Line 55: Line 56: public class ImportVmTemplateCommandTest { Line 57: Line 58: @ClassRule Line 59: public static MockConfigRule mcr = new MockConfigRule(); Why is this change necessary for this patch set? Line 60: Line 61: @Test Line 62: public void insufficientDiskSpace() { Line 63: final int lotsOfSpaceRequired = 1073741824; Line 163: Line 164: return command; Line 165: } Line 166: Line 167: private void spyOnGetMacPoolForDataCenter(ImportVmTemplateCommand command) { This should be inlined Line 168: doReturn(Mockito.mock(MacPoolManagerStrategy.class)).when(command).getMacPool(); Line 169: } Line 170: Line 171: private static void mockStorageDomains(ImportVmTemplateCommand command) { http://gerrit.ovirt.org/#/c/26800/45/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmStatic.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmStatic.java: Line 137: return this.vmtGuid; Line 138: } Line 139: Line 140: public void setVmtGuid(Guid value) { Line 141: No need for this empty line Line 142: this.vmtGuid = value; Line 143: } Line 144: Line 145: public boolean isInitialized() { -- To view, visit http://gerrit.ovirt.org/26800 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I779f4d031409fbc422077ddf291fa34f955f3206 Gerrit-PatchSet: 45 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
