Mike Kolesnik has posted comments on this change. Change subject: core: pool per DC ......................................................................
Patch Set 35: Code-Review-1 (7 comments) http://gerrit.ovirt.org/#/c/26799/35/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 33: static final String POOL_TO_BE_REMOVED_DOES_NOT_EXIST_EXCEPTION_MESSAGE = Line 34: "Trying to removed pool which does not exist."; Line 35: Line 36: //package local & non final for testing Line 37: static MacPoolPerDc INSTANCE = new MacPoolPerDc(); I've discussed this with Moti and we think that the logic for finding the correct mac pool should be in a separate class that will be tested by all the tests you wrote, and the MacPoolPerDc would be just a singleton wrapper for that class. This will eliminate need to expose the class internals just for the sake of tests, and allow easy testing. Line 38: Line 39: Line 40: private static final Log LOGGER = LogFactory.getLog(MacPoolPerDc.class); Line 41: protected final Map<Guid, MacPoolManagerStrategy> macPools = new HashMap<>(); Line 36: //package local & non final for testing Line 37: static MacPoolPerDc INSTANCE = new MacPoolPerDc(); Line 38: Line 39: Line 40: private static final Log LOGGER = LogFactory.getLog(MacPoolPerDc.class); Should be called log Line 41: protected final Map<Guid, MacPoolManagerStrategy> macPools = new HashMap<>(); Line 42: Line 43: private final ReentrantReadWriteLock lockObj = new ReentrantReadWriteLock(); Line 44: private boolean initialized = false; Line 118: private VM queryVm(Guid vmId) { Line 119: return DbFacade.getInstance().getVmDao().get(vmId); Line 120: } Line 121: Line 122: public MacPoolManagerStrategy poolForVmNic(VmNic vmNic) { Judging by the next patch and the comments there, this level of granularity is currently not necessary so this can be removed and will simplify the code here and the testing.. Line 123: return poolForVm(vmNic.getVmId()); Line 124: } Line 125: Line 126: private MacPoolManagerStrategy getPoolWithoutLocking(Guid macPoolId) { http://gerrit.ovirt.org/#/c/26799/35/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 34: import org.ovirt.engine.core.dao.VmDAO; Line 35: import org.ovirt.engine.core.dao.network.VmNicDao; Line 36: import org.ovirt.engine.core.utils.MockConfigRule; Line 37: Line 38: public class MacPoolPerDcTest { The tests are very cumbersome and hard to understand, please make an effort to write simple and concise tests that test exactly one scenario.. Tests are missing for the following scenarios (you should also decide what's the desired outcome in some of them): * Try to initialize twice * Each public method when the object is not initialized * Get existing pool by DC ID * Get existing pool for VM (by instance) * Get existing pool for VM (by instance) where the dc ID is not set on the VM * Get non-existing pool for VM (by instance) * Get existing pool for VM (by ID) * Get non-existing pool for VM (by ID) * Create a pool * Try to create a pool that's already registered * Modify an existing pool * Try to modify a pool that isn't registered * Delete an existing pool * Try to delete a pool that isn't registered Line 39: Line 40: public static final int MAX_MACS_COUNT_IN_POOL = 100000; Line 41: public static final boolean ALLOW_DUPLICATES = false; Line 42: public static final String MAC_POOL_RANGES = "00:1a:4a:15:c0:00-00:1a:4a:15:c0:ff"; Line 41: public static final boolean ALLOW_DUPLICATES = false; Line 42: public static final String MAC_POOL_RANGES = "00:1a:4a:15:c0:00-00:1a:4a:15:c0:ff"; Line 43: Line 44: @ClassRule Line 45: public static MockConfigRule mcr = new MockConfigRule( Not necessary anymore Line 46: mockConfig(ConfigValues.MacPoolRanges, MAC_POOL_RANGES), Line 47: mockConfig(ConfigValues.MaxMacsCountInPool, MAX_MACS_COUNT_IN_POOL), Line 48: mockConfig(ConfigValues.AllowDuplicateMacAddresses, ALLOW_DUPLICATES)); Line 49: Line 84: MacPoolPerDc.INSTANCE.poolForDataCenter(null); Line 85: } Line 86: Line 87: @Test() Line 88: public void testInitWasNotCalledForScope() throws Exception { I think the test name is misleading, it's testing that no pool is available for the given DC Line 89: MacPoolPerDc.INSTANCE.initialize(); Line 90: expectedException.expect(IllegalStateException.class); Line 91: expectedException.expectMessage(MacPoolPerDc.INEXISTENT_POOL_EXCEPTION_MESSAGE); Line 92: MacPoolPerDc.INSTANCE.poolForDataCenter(Guid.newGuid()); Line 206: errorCollector.checkThat("provided mac should be used in returned pool", MacPoolPerDc.INSTANCE.poolForVmNic(vmNic2).isMacInUse(macAddress2), is(true)); Line 207: } Line 208: Line 209: @Test Line 210: public void testCanRemovePoolWhichIsNotInUse() throws Exception { Not quite sure what this is testing exactly, there's no limitation on removing a pool whether it's "used" or not in this class context. Line 211: //mock data center Line 212: final MacPool macPool1 = createMacPool("00:1a:4a:15:c0:00", "00:1a:4a:15:c0:09"); Line 213: final StoragePool storagePool1 = createStoragePool(macPool1); Line 214: -- 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: 35 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: 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
