Martin Mucha has posted comments on this change. Change subject: core: MacPoolManager-->ScopedMacPoolManager ......................................................................
Patch Set 45: (11 comments) 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 Done 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 Done 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 Done 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 aft if IDE is not misleading me, there are places where VmInterfaceManager is created and setting MacPool is not needed. If that's true, setter cannot be pushed into constructor. Presence of "not always needed properties" indeed imply that VmInterfaceManager is badly designed, but that's different story. I just added required to calls where it's required. ~~ minimum change required to ... ~~ 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 Done 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 Done 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 Done 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 Done 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? Done 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 Done 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 Done 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
