Martin Mucha has posted comments on this change. Change subject: core: pool per DC ......................................................................
Patch Set 37: (32 comments) 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 pub Done 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.. Done 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.. Done 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 any Done 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" Done 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.. will cause message is not printed in some scenarios, yes, improbable ones, but on second thought in this case maybe not that improbable like for example OutOfMemoryError, which in this context was formerly fired, which lead to this feature. So for me, OOOME is real here. I've removed it, we can add it later back. 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" Done 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 still used in test, strengthened to package-local. 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 Done 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.. Done 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 it can not (should not), it would create overgrown method. 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. well *class name* should be named DataCenter. Variable name for class StoragePool should be storagePool. Doing it differently is against clean coding practices. 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... Done 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 added check to test if pool is returned (not null value). further testing is doable, for example via stubbing, but tested class would need to be altered to allow this. I think returning *some* pool should be sufficient. 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.. Done 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" Done 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.. Done 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 i you're right. It makes no sense. But you've insisted on this check removal, with argumentation, that this is caller's responsibility. We've removed it 'together' when I was in israel. It was present when this class was introduced ~ See createPoolWithoutLocking in patch 15 and it was removed in 34. I agree that this is this class responsibility, so I'm putting it back. 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 well, you can't. These methods are not and were never not part of pool api(especially original MacPoolManager). So I can do it, but only after we agree on how we want to do it: a) hardcore reflection b) introduce methods allowing stubbing and run this test upon stub c) introduce methods for getting duplicate setting and ranges setting etc. to MacPoolManagerStrategy and it's implementations. d) ??? 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. Done 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.. Done 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 po Done 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.. Done 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.. this is partly covered by one of first test, but I've added check for poolForDataCenter verifying that this pool does not exist. I don't know how to assert exceptions which does not block execution, which cannot be done using Rule not expected attribute, so I had to resort to try/catch. 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' Done 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 Done 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 o Done 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 Done 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.. Done 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... Done 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... Done 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... Done 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
