Mike Kolesnik has posted comments on this change. Change subject: core: pool per DC ......................................................................
Patch Set 37: (32 comments) Sorry that I'm publishing in an older patchset, but it would be too time consuming to copy each one to the newest patch set. I checked against latest (#41) and all are still relevant. http://gerrit.ovirt.org/#/c/26799/37/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacPoolPerDc.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacPoolPerDc.java: Line 18: import org.ovirt.engine.core.utils.MacAddressRangeUtils; Line 19: import org.ovirt.engine.core.utils.log.Log; Line 20: import org.ovirt.engine.core.utils.log.LogFactory; Line 21: Line 22: public class MacPoolPerDc { Please use the new AutoClosableLock wrapper when locking in the various public methods. Line 23: Line 24: static final String INEXISTENT_POOL_EXCEPTION_MESSAGE = "Coding error, pool for requested GUID does not exist"; Line 25: static final String NOT_INITIALIZED_EXCEPTION_MESSAGE = "This instance was not yet initialized."; Line 26: static final String CANNOT_REMOVE_POOL_EXCEPTION_MESSAGE = "Cannot remove pool which is still in use."; Line 27: static final String POOL_TO_BE_REMOVED_DOES_NOT_EXIST_EXCEPTION_MESSAGE = Line 28: "Trying to removed pool which does not exist."; Line 29: Line 30: private static final Log log = LogFactory.getLog(MacPoolPerDc.class); Line 31: protected final Map<Guid, MacPoolManagerStrategy> macPools = new HashMap<>(); Should be private.. Line 32: Line 33: private final ReentrantReadWriteLock lockObj = new ReentrantReadWriteLock(); Line 34: private boolean initialized = false; Line 35: Line 33: private final ReentrantReadWriteLock lockObj = new ReentrantReadWriteLock(); Line 34: private boolean initialized = false; Line 35: Line 36: //package local for testing Line 37: MacPoolPerDc() { Should be public.. Line 38: } Line 39: Line 40: public final void initialize() { Line 41: lockObj.writeLock().lock(); Line 41: lockObj.writeLock().lock(); Line 42: Line 43: try { Line 44: if (initialized) { Line 45: log.error("Trying to initialized " + getClass().getName() + " multiple times."); No need to print getClass().getName() (since it's printed by the logger anyhow) Line 46: return; Line 47: } Line 48: Line 49: final List<MacPool> macPools = getMacPoolDao().getAll(); Line 50: for (MacPool macPool : macPools) { Line 51: initializeMacPool(macPool); Line 52: } Line 53: initialized = true; Line 54: log.info("MAC pool successfully initialized"); No need to print "MAC pool" Line 55: } catch(Throwable t) { Line 56: log.error("MAC pool not initialized."); Line 57: throw t; Line 58: } finally { Line 51: initializeMacPool(macPool); Line 52: } Line 53: initialized = true; Line 54: log.info("MAC pool successfully initialized"); Line 55: } catch(Throwable t) { I think Throwable is too broad here.. I'd catch perhaps RuntimeException as don't see a need to catch anything broader. Line 56: log.error("MAC pool not initialized."); Line 57: throw t; Line 58: } finally { Line 59: lockObj.writeLock().unlock(); Line 52: } Line 53: initialized = true; Line 54: log.info("MAC pool successfully initialized"); Line 55: } catch(Throwable t) { Line 56: log.error("MAC pool not initialized."); No need to print "MAC pool" Line 57: throw t; Line 58: } finally { Line 59: lockObj.writeLock().unlock(); Line 60: } Line 83: lockObj.readLock().unlock(); Line 84: } Line 85: } Line 86: Line 87: protected Guid getMacPoolId(Guid dataCenterId) { Should be private Line 88: final StoragePool storagePool = DbFacade.getInstance().getStoragePoolDao().get(dataCenterId); Line 89: return storagePool == null ? null : storagePool.getMacPoolId(); Line 90: } Line 91: Line 88: final StoragePool storagePool = DbFacade.getInstance().getStoragePoolDao().get(dataCenterId); Line 89: return storagePool == null ? null : storagePool.getMacPoolId(); Line 90: } Line 91: Line 92: private VM queryVm(Guid vmId) { This method is unused Line 93: return DbFacade.getInstance().getVmDao().get(vmId); Line 94: } Line 95: Line 96: private MacPoolManagerStrategy getPoolWithoutLocking(Guid macPoolId) { Line 120: MacPoolManagerStrategy poolForScope = createAndInitPool(macPool); Line 121: macPools.put(macPool.getId(), poolForScope); Line 122: return poolForScope; Line 123: } Line 124: This method can be inlined.. Line 125: private MacPoolManagerStrategy createAndInitPool(MacPool macPool) { Line 126: MacPoolManagerStrategy poolForScope = new MacPoolManagerRanges(macPoolToRanges(macPool), Line 127: macPool.isAllowDuplicateMacAddresses()); Line 128: poolForScope.initialize(); Line 147: lockObj.writeLock().unlock(); Line 148: } Line 149: } Line 150: Line 151: private Collection<LongRange> macPoolToRanges(MacPool macPool) { Can optionally be inlined Line 152: final DisjointRanges disjointRanges = new DisjointRanges(); Line 153: for (MacRange macRange : macPool.getRanges()) { Line 154: disjointRanges.addRange(MacAddressRangeUtils.macToLong(macRange.getMacFrom()), Line 155: MacAddressRangeUtils.macToLong(macRange.getMacTo())); http://gerrit.ovirt.org/#/c/26799/37/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/MacPoolPerDcTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/MacPoolPerDcTest.java: Line 36: Line 37: @Rule Line 38: public ErrorCollector errorCollector = new ErrorCollector(); Line 39: private MacPool macPoolA; Line 40: private StoragePool storagePoolA; Variable name should be dataCenter. Line 41: private VmNic vmNicA; Line 42: Line 43: @Before Line 44: public void setUp() throws Exception { Line 37: @Rule Line 38: public ErrorCollector errorCollector = new ErrorCollector(); Line 39: private MacPool macPoolA; Line 40: private StoragePool storagePoolA; Line 41: private VmNic vmNicA; Why A suffix? There's no B or something... Line 42: Line 43: @Before Line 44: public void setUp() throws Exception { Line 45: final DbFacade dbFacadeMock = mock(DbFacade.class); Line 79: public void testPoolOfGivenGuidExist() { Line 80: final MacPoolPerDc pool = new MacPoolPerDc(); Line 81: mockStoragePools(storagePoolA); Line 82: pool.initialize(); Line 83: assertThat(pool.getMacPoolId(storagePoolA.getId()), notNullValue()); You should be testing the public methods, so the correct method to call is poolForDataCenter Line 84: } Line 85: Line 86: @Test Line 87: public void testNicIsCorrectlyAllocatedInScopedPool() throws Exception { Line 91: Line 92: final MacPoolPerDc pool = new MacPoolPerDc(); Line 93: pool.initialize(); Line 94: assertThat("scoped pool for this nic should exist", pool.poolForDataCenter(storagePoolA.getId()), notNullValue()); Line 95: assertThat("provided mac should be used in returned pool", pool.poolForDataCenter(storagePoolA.getId()).isMacInUse(vmNicA.getMacAddress()), is(true)); These 2 lines are way too long.. Please format them to be within the 120 length limit (while still being readable) Line 96: } Line 97: Line 98: @Test Line 99: public void testCreatePool() throws Exception { Line 101: pool.initialize(); Line 102: Line 103: mockStoragePools(storagePoolA); Line 104: pool.createPool(macPoolA); Line 105: assertThat("scoped pool for this nic should exist", pool.poolForDataCenter(storagePoolA.getId()), notNullValue()); I think you meant "DC" in the message, not "nic" Line 106: } Line 107: Line 108: @Test Line 109: public void testCreatePoolWhichExists() throws Exception { Line 109: public void testCreatePoolWhichExists() throws Exception { Line 110: final MacPoolPerDc pool = new MacPoolPerDc(); Line 111: Line 112: mockStoragePools(storagePoolA); Line 113: mockGettingAllMacPools(macPoolA); No need for this.. Line 114: pool.initialize(); Line 115: Line 116: pool.createPool(macPoolA); Line 117: //should not fail. Line 113: mockGettingAllMacPools(macPoolA); Line 114: pool.initialize(); Line 115: Line 116: pool.createPool(macPoolA); Line 117: //should not fail. I don't think this makes sense, it would just silently replace an already initialized manager (which could be referenced in other places in the code), potentially hiding a bug.. I think it would make more sense to throw some exception in this case.. Line 118: } Line 119: Line 120: @Test Line 121: public void testModifyOfExistingMacPool() throws Exception { Line 126: pool.initialize(); Line 127: Line 128: macPoolA.setAllowDuplicateMacAddresses(true); Line 129: pool.modifyPool(macPoolA); Line 130: //should not fail. You should probably check that the new pool strategy is allowing duplicate mac addresses (even possible to check that the old one doesn't before you modify it). I would additionally use different ranges and check that too, to be sure the pool was modified. Line 131: } Line 132: Line 133: @Test Line 134: public void testModifyOfNotExistingMacPool() throws Exception { Line 134: public void testModifyOfNotExistingMacPool() throws Exception { Line 135: final MacPoolPerDc pool = new MacPoolPerDc(); Line 136: Line 137: mockStoragePools(storagePoolA); Line 138: mockGettingAllMacPools(macPoolA); No need for this. Line 139: pool.initialize(); Line 140: Line 141: macPoolA.setAllowDuplicateMacAddresses(true); Line 142: Line 137: mockStoragePools(storagePoolA); Line 138: mockGettingAllMacPools(macPoolA); Line 139: pool.initialize(); Line 140: Line 141: macPoolA.setAllowDuplicateMacAddresses(true); This isn't necessary either.. Line 142: Line 143: expectedException.expect(IllegalStateException.class); Line 144: expectedException.expectMessage(MacPoolPerDc.INEXISTENT_POOL_EXCEPTION_MESSAGE); Line 145: pool.modifyPool(createMacPool(null, null)); Line 156: pool.removePool(macPoolA.getId()); Line 157: Line 158: Line 159: mockStoragePools(); Line 160: mockGettingAllMacPools(); I don't believe that in this situation you need to mock anything, if the pool was removed then the exception will be thrown anyway.. Line 161: Line 162: expectedException.expect(IllegalStateException.class); Line 163: expectedException.expectMessage(MacPoolPerDc.INEXISTENT_POOL_EXCEPTION_MESSAGE); Line 164: pool.poolForDataCenter(storagePoolA.getId()); Line 160: mockGettingAllMacPools(); Line 161: Line 162: expectedException.expect(IllegalStateException.class); Line 163: expectedException.expectMessage(MacPoolPerDc.INEXISTENT_POOL_EXCEPTION_MESSAGE); Line 164: pool.poolForDataCenter(storagePoolA.getId()); You should assert before the remove that this pool existed.. Line 165: } Line 166: Line 167: @Test Line 168: public void testRemoveOfInexistentMacPool() throws Exception { Line 168: public void testRemoveOfInexistentMacPool() throws Exception { Line 169: final MacPoolPerDc pool = new MacPoolPerDc(); Line 170: pool.initialize(); Line 171: pool.removePool(macPoolA.getId()); Line 172: //nothing to test, should not fail. You should assert before the removal that the pool doesn't exist.. Line 173: } Line 174: Line 175: private StoragePool createStoragePool(MacPool macPool) { Line 176: Line 199: protected void mockGettingAllMacPools(MacPool... macPool) { Line 200: when(macPoolDaoMock.getAll()).thenReturn(Arrays.asList(macPool)); Line 201: } Line 202: Line 203: protected VmNic createVmNicA() { Not sure why suffix 'A' Line 204: final VmNic vmNic = new VmNic(); Line 205: vmNic.setMacAddress("00:1a:4a:15:c0:fe"); Line 206: return vmNic; Line 207: } Line 205: vmNic.setMacAddress("00:1a:4a:15:c0:fe"); Line 206: return vmNic; Line 207: } Line 208: Line 209: protected void mockStoragePools(StoragePool... storagePool) { I don't see any case where ellipsis is necessary, you should mock just call for one StoragePool object. Line 210: when(storagePoolDaoMock.getAll()).thenReturn(Arrays.asList(storagePool)); Line 211: Line 212: for (StoragePool pool : storagePool) { Line 213: //mock obtaining storagepool by id. Line 208: Line 209: protected void mockStoragePools(StoragePool... storagePool) { Line 210: when(storagePoolDaoMock.getAll()).thenReturn(Arrays.asList(storagePool)); Line 211: Line 212: for (StoragePool pool : storagePool) { Not sure that this code is necessary for all 7 tests using it, seems that only 3 are relying on it and the rest can live without so it might be worthwhile to extract this to a seperate method and only call it from the relevant tests. After further checking, the mocking "when(storagePoolDaoMock.getAll()).thenReturn(Arrays.asList(storagePool));" is utterly unnecessary since this isn't called by that class, so you can call this method only when necessary. Line 213: //mock obtaining storagepool by id. Line 214: when(storagePoolDaoMock.get(eq(pool.getId()))).thenReturn(pool); Line 215: } Line 216: http://gerrit.ovirt.org/#/c/26799/37/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/MacPoolPerDcUsedBeforeInitializedTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/MacPoolPerDcUsedBeforeInitializedTest.java: Line 3: import org.junit.Rule; Line 4: import org.junit.Test; Line 5: import org.junit.rules.ExpectedException; Line 6: Line 7: public class MacPoolPerDcUsedBeforeInitializedTest { This should all be part of the original test class, there's no good reason to split it into two classes. Line 8: @Rule Line 9: public ExpectedException expectedException = ExpectedException.none(); Line 10: Line 11: private void expectNotInitializedException() { Line 12: expectedException.expect(IllegalStateException.class); Line 13: expectedException.expectMessage(MacPoolPerDc.NOT_INITIALIZED_EXCEPTION_MESSAGE); Line 14: } Line 15: Line 16: @Test() Unnecessary parentheses.. Line 17: public void testPoolForDataCenterMethod() throws Exception { Line 18: expectNotInitializedException(); Line 19: new MacPoolPerDc().poolForDataCenter(null); Line 20: } Line 18: expectNotInitializedException(); Line 19: new MacPoolPerDc().poolForDataCenter(null); Line 20: } Line 21: Line 22: @Test() Same here... Line 23: public void testCreatePoolMethod() throws Exception { Line 24: expectNotInitializedException(); Line 25: new MacPoolPerDc().createPool(null); Line 26: } Line 24: expectNotInitializedException(); Line 25: new MacPoolPerDc().createPool(null); Line 26: } Line 27: Line 28: @Test() Same here... Line 29: public void testModifyPoolMethod() throws Exception { Line 30: expectNotInitializedException(); Line 31: new MacPoolPerDc().modifyPool(null); Line 32: } Line 30: expectNotInitializedException(); Line 31: new MacPoolPerDc().modifyPool(null); Line 32: } Line 33: Line 34: @Test() Same here... Line 35: public void testRemovePoolMethod() throws Exception { Line 36: expectNotInitializedException(); Line 37: new MacPoolPerDc().removePool(null); Line 38: } -- To view, visit http://gerrit.ovirt.org/26799 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If522a0b0d5f810281bed010b46b5242f7dbdcc29 Gerrit-PatchSet: 37 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <[email protected]> Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: Mike Kolesnik <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
