Martin Mucha has posted comments on this change. Change subject: core: pool per DC ......................................................................
Patch Set 35: (6 comments) answers. All this requires fine amount of work, so I'll be working on it for some time. I'm presenting answers for some disputable comments, so we can agree on whether we REALLY want do it that way. 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 c ok, I'll do it. Just to be correct: this will eliminate need to allow access to INSTANCE field. Other remains exactly same. 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 Done 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 Done 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 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 Done 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 Done 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 remov I have no idea what do you mean by "in this class context" means. MacPoolPerDc reads data from db and initializes it's content upon it. And from db perspective is not possible to delete used pool. So since this class is not puppet of other classes it is THIS CLASS responsibility to validate this, caller. Maybe I'm getting it. Did you (again) mean to move responsibility to do this checking in canDoAction of EACH goddamn Command class(flawed OO design)? 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: 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
