Martin Mucha has posted comments on this change. Change subject: core: pool per DC ......................................................................
Patch Set 37: (3 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 51: initializeMacPool(macPool); Line 52: } Line 53: initialized = true; Line 54: log.info("MAC pool successfully initialized"); Line 55: } catch(Throwable t) { > I thought you took care of that OOME :) I did (at least I think so), but evidently it could occur here, from that perspective this would make sense. But it's changed to RuntimeException. Line 56: log.error("MAC pool not initialized."); Line 57: throw t; Line 58: } finally { Line 59: lockObj.writeLock().unlock(); 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; > Then by that logic you shouldn't suffix the mock fields with Mock. great. So lets forget about 'logic' and talk about clean coding: SUFFIX or PREFIX is ok. Like: Car mineCar; Car carFromRental. So "Mock" suffix means, that this is mock implementation of this interface. It's some sort of specifier. This is fine. Primitives and primitive wrappers are too generic objects. This does not apply for them. The only problem here is naming car a tree, like: Car tree; It's fine of course, for compiller, but that's the reason why we do not use "a,b,c,d,e" as variable names, although this was pretty popular in 1980. This actions make whole product less readable and more buggy because even authors do not understand it. I've saw such code once, this is not right way. As I suggested; if you tend to name it StoragePool datacenter, you should refactor whole class. It's fucking easy, I can do it. Actually someone should do that so we can drop mapping DataCenter<-->StoragePool from our brains. —— Now, if this is some sort of mental excercise just for fun, it's ok. If you meant this seriously, please, read something about "clean coding" subject. Line 41: private VmNic vmNicA; Line 42: Line 43: @Before Line 44: public void setUp() throws Exception { Line 126: pool.initialize(); Line 127: Line 128: macPoolA.setAllowDuplicateMacAddresses(true); Line 129: pool.modifyPool(macPoolA); Line 130: //should not fail. > You can allocate new mac address from the pool and then try to allocate it - ad duplicity: done - ad ranges: allocating outside of range is allowed. But if we're allowing such correlations as a proof, I could abuse method 'allocateNewMac' to test range change. Line 131: } Line 132: Line 133: @Test Line 134: public void testModifyOfNotExistingMacPool() throws Exception { -- 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
