Martin Mucha has posted comments on this change. Change subject: core: MacPoolManager strategy, alternative implementation, test for benchmarks ......................................................................
Patch Set 6: (55 comments) answers. http://gerrit.ovirt.org/#/c/26405/6/backend/manager/modules/bll/pom.xml File backend/manager/modules/bll/pom.xml: Line 19: </properties> Line 20: Line 21: <dependencies> Line 22: Line 23: <dependency> > how that is being used? RFE requested improvement. No specific acceptance criteria was set, so this was pushed to allow RFE owner to reproduce mine measurements and say whether he's OK with result. So this was waiting here for owner of RFE check followed by mine removal. I'm removing it now. Line 24: <groupId>com.github.stephenc</groupId> Line 25: <artifactId>jamm</artifactId> Line 26: <version>0.2.5</version> Line 27: <scope>test</scope> http://gerrit.ovirt.org/#/c/26405/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacPoolManagerRanges.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacPoolManagerRanges.java: Line 17: import org.ovirt.engine.core.utils.MacAddressRangeUtils; Line 18: import org.ovirt.engine.core.utils.log.Log; Line 19: import org.ovirt.engine.core.utils.log.LogFactory; Line 20: Line 21: public class MacPoolManagerRanges implements MacPoolManagerStrategy{ > Please use the standard oVirt formatter no problem, I can change it. But this was actually used in original implementation, this very same thing is used heavily through project. So what's the correct way? Line 22: Line 23: private static final Log log = LogFactory.getLog(MacPoolManagerRanges.class); Line 24: Line 25: private final int maxMacsInPool; Line 36: this.allowDuplicates = allowDuplicates; Line 37: } Line 38: Line 39: @Override Line 40: public void initialize() { > can it be called if it initialized already? no, I've fixed it in following commit, I'll move it in here. Line 41: lockObj.writeLock().lock(); Line 42: try { Line 43: log.infoFormat("Start initializing "+getClass().getSimpleName()); Line 44: Line 41: lockObj.writeLock().lock(); Line 42: try { Line 43: log.infoFormat("Start initializing "+getClass().getSimpleName()); Line 44: Line 45: this.macsStorage = initRanges(rangesString); > Why address with "this"? why not? Does it worsen code base or emphasize that this is field not local variable.?This is 2014 not 1960, it's about readability not text compression. Line 46: Line 47: Line 48: List<VmNic> interfaces = getVmNicInterfacesFromDB(); Line 49: Line 58: lockObj.writeLock().unlock(); Line 59: } Line 60: } Line 61: Line 62: MacsStorage initRanges(String rangesString) { > 1. please make it private Done Line 63: List<Pair<Long, Long>> rangesBoundaries = MacAddressRangeUtils.rangesStringToRangeBoundaries(rangesString); Line 64: int macsToCreate = maxMacsInPool; Line 65: MacsStorage macsStorage = new MacsStorage(this.allowDuplicates); Line 66: for (Pair<Long, Long> rangesBoundary : rangesBoundaries) { Line 74: } Line 75: } Line 76: Line 77: //to allow testing; may be fixed after DI is used. Line 78: List<VmNic> getVmNicInterfacesFromDB() { > I'd prefer passing DAO through the constructor. That would allow mocking th not me. Passing dependencies through constructor spells: a) ugly overgrown constructors b) when ugly overgrown classes are used (75% of all classes ever written) then you have to instantiate class with references which aren't even needed for your task and maybe you even not possess them in context of caller, so you have to obtain them even if you do not need them. Alternative is countless hordes of constructor and no one is sure which one to use for his intention. --> passing through constructor is nonsense. Use of DI is proper way. I'm making this method private. Line 79: return DbFacade.getInstance().getVmNicDao().getAll(); Line 80: } Line 81: Line 82: //to allow testing; may be fixed after DI is used. Line 79: return DbFacade.getInstance().getVmNicDao().getAll(); Line 80: } Line 81: Line 82: //to allow testing; may be fixed after DI is used. Line 83: void logMacPoolEmpty() { > AuditLogDirectorDelegator was added in order to enable mocking the logging same as above. After tests removal noone calls this, so I'm making it private. Line 84: AuditLogableBase logable = new AuditLogableBase(); Line 85: AuditLogDirector.log(logable, AuditLogType.MAC_POOL_EMPTY); Line 86: } Line 87: Line 86: } Line 87: Line 88: @Override Line 89: public String allocateNewMac() { Line 90: lockObj.writeLock().lock(); > Or you could argue that it belongs in MacPoolManager, regardless of which s dropped original implementation along with verification tests. So sole implementation only now. If super-needed, I can drop that strategy again. (remember, it was needed to proper verify new implementation and prove better cpu and memory complexity). Line 91: try { Line 92: return allocateNewMacImpl(1)[0]; Line 93: } finally { Line 94: lockObj.writeLock().unlock(); Line 94: lockObj.writeLock().unlock(); Line 95: } Line 96: } Line 97: Line 98: private String[] allocateNewMacImpl(int numberOfMacs) { > 1. Please avoid using arrays, use collections instead. ad1. ok ad2. allocateNewMacsWithoutLocking. Line 99: if (!initialized) { Line 100: logInitializationError("Failed to allocate new Mac address."); Line 101: throw new VdcBLLException(VdcBllErrors.MAC_POOL_NOT_INITIALIZED); Line 102: } Line 94: lockObj.writeLock().unlock(); Line 95: } Line 96: } Line 97: Line 98: private String[] allocateNewMacImpl(int numberOfMacs) { > Why not plural method name? plural should be there(it is now), but namely it must be clear from that name, that it's not same method as allocateNewMacImpl since there's missing locking. Some developers tend to do shortcuts: "why the hell this method is not public when I need it"? So the name has to be (for shortcutters) in that form, that it's obvious to them, that they actually do not want to use this method. Line 99: if (!initialized) { Line 100: logInitializationError("Failed to allocate new Mac address."); Line 101: throw new VdcBLLException(VdcBllErrors.MAC_POOL_NOT_INITIALIZED); Line 102: } Line 95: } Line 96: } Line 97: Line 98: private String[] allocateNewMacImpl(int numberOfMacs) { Line 99: if (!initialized) { > The check is not initialized is redundant here. That is checked in the call it is? I cannot see it. But even if it is. Every public method has to have this checking and since our 2 methods just delegates to this method, to avoid duplicity this check is done here. Of course, this is wrong. Locking and initialization checking is a cross-cutting concern and should not be present here. If you approve that I'll be moving it gladly to appropriate location (proxy/decorator). Line 100: logInitializationError("Failed to allocate new Mac address."); Line 101: throw new VdcBLLException(VdcBllErrors.MAC_POOL_NOT_INITIALIZED); Line 102: } Line 103: if (macsStorage.getAvailableMacsCount() < numberOfMacs) { //TODO MM: allocate just those remaining or none? Line 99: if (!initialized) { Line 100: logInitializationError("Failed to allocate new Mac address."); Line 101: throw new VdcBLLException(VdcBllErrors.MAC_POOL_NOT_INITIALIZED); Line 102: } Line 103: if (macsStorage.getAvailableMacsCount() < numberOfMacs) { //TODO MM: allocate just those remaining or none? > I guess that should in MacsStorage.allocateAvailableMac Done Line 104: throw new VdcBLLException(VdcBllErrors.MAC_POOL_NO_MACS_LEFT); Line 105: } Line 106: Line 107: long[] macs = macsStorage.allocateAvailableMac(numberOfMacs); Line 105: } Line 106: Line 107: long[] macs = macsStorage.allocateAvailableMac(numberOfMacs); Line 108: Arrays.sort(macs); Line 109: if (macsStorage.noAvailableMacs()) { > I guess that should in MacsStorage.allocateAvailableMac seems like it. But responsibility to log something when last item from storage is used seems to be outside of scope of storage. I don't understand code contract of mac pools much, I'm just refactoring it. This is just reshuffled preexisting code. With previous check you're right, but this I think belongs here. Line 110: logMacPoolEmpty(); Line 111: } Line 112: Line 113: return MacAddressRangeUtils.macAddressToString(macs); Line 150: loggable.addCustomValue("Message", message); Line 151: AuditLogDirector.log(loggable, AuditLogType.MAC_ADDRESSES_POOL_NOT_INITIALIZED); Line 152: } Line 153: Line 154: /** > Please move javadoc to the interface Done Line 155: * Add given MAC address if possible. Line 156: * Add user define mac address Function return false if the mac is in use Line 157: * @return true if MAC was added successfully, and false if the MAC is in use and Line 158: * {@link org.ovirt.engine.core.common.config.ConfigValues#AllowDuplicateMacAddresses} is set to false Line 160: @Override Line 161: public boolean addMac(String mac) { Line 162: lockObj.writeLock().lock(); Line 163: try { Line 164: boolean added = this.macsStorage.useMac(MacAddressRangeUtils.macStringToLong(mac)); > Why address with "this"? same as above. Line 165: if (macsStorage.noAvailableMacs()) { Line 166: logMacPoolEmpty(); Line 167: } Line 168: return added; Line 173: } Line 174: } Line 175: Line 176: /** Line 177: * Add given MAC address, regardless of it being in use. > Please move javadoc to the interface Done Line 178: * Line 179: */ Line 180: @Override Line 181: public void forceAddMac(String mac) { Line 180: @Override Line 181: public void forceAddMac(String mac) { Line 182: lockObj.writeLock().lock(); Line 183: try { Line 184: this.macsStorage.useMacNoDuplicityCheck(MacAddressRangeUtils.macStringToLong(mac)); > Why address with "this"? same Line 185: if (macsStorage.noAvailableMacs()) { Line 186: logMacPoolEmpty(); Line 187: } Line 188: } finally { Line 193: @Override Line 194: public boolean isMacInUse(String mac) { Line 195: lockObj.readLock().lock(); Line 196: try { Line 197: return this.macsStorage.isMacInUse(MacAddressRangeUtils.macStringToLong(mac)); > Why address with "this"? same Line 198: Line 199: } finally { Line 200: lockObj.readLock().unlock(); Line 201: } Line 202: } Line 203: Line 204: @Override Line 205: public void freeMacs(List<String> macs) { //TODO MM: how about some duplicities here? Release it twice or what? Line 206: if (!macs.isEmpty()) { > Why check if not empty? not to obtain lock without a reason. Ok we're not very concurrent environment, but still. Getting a write lock when I can say that I do not actually need it is wrong Line 207: lockObj.writeLock().lock(); Line 208: try { Line 209: if (!initialized) { Line 210: logInitializationError("Failed to free MAC addresses."); Line 207: lockObj.writeLock().lock(); Line 208: try { Line 209: if (!initialized) { Line 210: logInitializationError("Failed to free MAC addresses."); Line 211: } > So even if not initialized it will try to free the MACs? this is just refactoring. This was present in our code base. I was fixing these issues in following commits. I'll move it into this patch set. Line 212: for (String mac : macs) { Line 213: macsStorage.freeMac(MacAddressRangeUtils.macStringToLong(mac)); Line 214: } Line 215: Line 219: } Line 220: } Line 221: Line 222: /** Line 223: * Allocates mac addresses according to the input, sorting them in ascending order > Please move javadoc to the interface Done Line 224: * Line 225: * @param numberOfAddresses Line 226: * The number of MAC addresses to allocate Line 227: * @return The list of MAC addresses, sorted in ascending order Line 229: @Override Line 230: public List<String> allocateMacAddresses(int numberOfAddresses) { Line 231: lockObj.writeLock().lock(); Line 232: try { Line 233: String[] macAddresses = this.allocateNewMacImpl(numberOfAddresses); > Why address with "this"? same Line 234: return Arrays.asList(macAddresses); Line 235: } finally { Line 236: lockObj.writeLock().unlock(); Line 237: } Line 236: lockObj.writeLock().unlock(); Line 237: } Line 238: } Line 239: Line 240: private static class MacsStorage { > The class has a lot of functionality, it deserves to be an independent clas Done Line 241: private final Boolean allowDuplicates; Line 242: private List<Range> ranges = new LinkedList<>(); Line 243: private ObjectCounter<Long> customMacs; Line 244: Line 237: } Line 238: } Line 239: Line 240: private static class MacsStorage { Line 241: private final Boolean allowDuplicates; > What is the meaning of null here? Done Line 242: private List<Range> ranges = new LinkedList<>(); Line 243: private ObjectCounter<Long> customMacs; Line 244: Line 245: public MacsStorage(Boolean allowDuplicates) { Line 246: this.allowDuplicates = allowDuplicates; Line 247: customMacs = new ObjectCounter<>(this.allowDuplicates); Line 248: } Line 249: Line 250: public long addRange(long first, long second, int maxMacsInPool) { > IMHO start/end or min/max are better names than first/second rangeStart/rangeEnd. min/max, start/end does not work for me. Line 251: Line 252: long i = first; Line 253: int numberOfNonMultiCasts = 0; Line 254: Line 258: } Line 259: } Line 260: Line 261: long size = i - first; Line 262: if (size > Integer.MAX_VALUE) { > Seems to me like a limitation that doesn't exist (theoretically) in the ori former solution used Collections. http://docs.oracle.com/javase/7/docs/api/java/util/Collection.html#size() So limitation existed. Bitsets are fine. Util you need to count number of usage of each MAC without knowing maximum number of usage count. Then you have to use counter(int) for each mac. Lets be honest. Engine would not survive this. 2^32-1 * 4B ~~ 8.5GB. That will not work. So we are 'limited' by capability of having only 2^32-1 of macs. And we are not at the same time. Also using byte as a type for counter would not work and <someone> already expressed that 255 duplicity usages at a same time is not enough. I think this "limitation" is just a fantasy. Line 263: throw new IllegalArgumentException("Not supported that wide ranges(" + size + ")."); Line 264: } Line 265: Line 266: if (numberOfNonMultiCasts > 0) { Line 277: testForMultiCastMac(mac); Line 278: Line 279: Range range = getRange(mac); Line 280: if (range == null) { Line 281: return this.customMacs.add(mac); > Why address with "this"? same Line 282: } else { Line 283: return range.use(mac, allowDuplicates); Line 284: } Line 285: } Line 291: public boolean isMacInUse(long mac) { Line 292: testForMultiCastMac(mac); Line 293: Line 294: Range range = getRange(mac); Line 295: return range != null ? range.isAllocated(mac) : this.customMacs.contains(mac); > please swap the condition to be a positive Done Line 296: } Line 297: Line 298: public boolean freeMac(long mac) { Line 299: testForMultiCastMac(mac); Line 291: public boolean isMacInUse(long mac) { Line 292: testForMultiCastMac(mac); Line 293: Line 294: Range range = getRange(mac); Line 295: return range != null ? range.isAllocated(mac) : this.customMacs.contains(mac); > Why address with "this"? same Line 296: } Line 297: Line 298: public boolean freeMac(long mac) { Line 299: testForMultiCastMac(mac); Line 298: public boolean freeMac(long mac) { Line 299: testForMultiCastMac(mac); Line 300: Line 301: Range range = getRange(mac); Line 302: if (range != null) { > please swap the condition to be a positive Done Line 303: return range.freeMac(mac); Line 304: } else { Line 305: return this.customMacs.remove(mac); Line 306: } Line 301: Range range = getRange(mac); Line 302: if (range != null) { Line 303: return range.freeMac(mac); Line 304: } else { Line 305: return this.customMacs.remove(mac); > Why address with "this"? same Line 306: } Line 307: } Line 308: Line 309: public boolean noAvailableMacs() { Line 315: return true; Line 316: } Line 317: Line 318: public long[] allocateAvailableMac(int numberOfMacs) { Line 319: return getRangeWithAvailableMac().allocateMac(numberOfMacs); > What if getRangeWithAvailableMac() returns a range with a single MAC availa weird, in my code there's check for that. So maybe it will be there in following patchset also. Line 320: } Line 321: Line 322: private Range getRangeWithAvailableMac() { Line 323: for (Range range : ranges) { Line 337: } Line 338: Line 339: private void testForMultiCastMac(long mac) { Line 340: if (MacAddressRangeUtils.macHasMultiCastBitSet(mac)) { Line 341: throw new IllegalArgumentException("You're trying to work with multicast address."); //TODO MM: should this be prohibited? > A specific exception should be created for that case, otherwise IllegalArgu it should? I mean reaching this line means failed validation on gui ... Or improper usage of rest. Line 342: } Line 343: } Line 344: Line 345: private Range getRange(long mac) { Line 341: throw new IllegalArgumentException("You're trying to work with multicast address."); //TODO MM: should this be prohibited? Line 342: } Line 343: } Line 344: Line 345: private Range getRange(long mac) { > findIncludingRange is more descriptive here Done Line 346: for (Range range : ranges) { Line 347: if (range.offset <= mac && (range.offset + range.size) >= mac) { Line 348: return range; Line 349: } Line 343: } Line 344: Line 345: private Range getRange(long mac) { Line 346: for (Range range : ranges) { Line 347: if (range.offset <= mac && (range.offset + range.size) >= mac) { > I'd move it to Range.contains method Done Line 348: return range; Line 349: } Line 350: } Line 351: return null; Line 343: } Line 344: Line 345: private Range getRange(long mac) { Line 346: for (Range range : ranges) { Line 347: if (range.offset <= mac && (range.offset + range.size) >= mac) { > Second that Done Line 348: return range; Line 349: } Line 350: } Line 351: return null; Line 350: } Line 351: return null; Line 352: } Line 353: Line 354: private static class Range { > 1. the class independence + test goes here too ad 2. ok, but that would mean interface and two implementations. And after that, there will be discussion about class diagram complexity, and about lots of code which can be removed by just one simple boolean flag. There's a trouble with disallowing duplicates. Preexisting code contract violates it. If you allow duplicates and create multiple of them, then turn engine off, disable duplicates, and turn engine on — well, your pool contains duplicates. See org.ovirt.engine.core.bll.network.MacPoolManagerRanges#forceAddMac that's the reason why that duplicity allowance setting is not on field. There are methods which are allowed to violate that setting. Line 355: private final long offset; Line 356: private final int size; Line 357: private final int[] macUsageCount; Line 358: private int _notUsedCount; Line 353: Line 354: private static class Range { Line 355: private final long offset; Line 356: private final int size; Line 357: private final int[] macUsageCount; > Isn't ObjectCount suitable here? no. ObjectCount is wrapper for HashMap. Using it was main reason of this bug/change — to stop using HashMap (~memory ineffectivity). Line 358: private int _notUsedCount; Line 359: Line 360: private NotUsedMacs notUsedMacs; Line 361: Line 354: private static class Range { Line 355: private final long offset; Line 356: private final int size; Line 357: private final int[] macUsageCount; Line 358: private int _notUsedCount; > please avoid using "_" Done Line 359: Line 360: private NotUsedMacs notUsedMacs; Line 361: Line 362: public interface NotUsedMacs { Line 358: private int _notUsedCount; Line 359: Line 360: private NotUsedMacs notUsedMacs; Line 361: Line 362: public interface NotUsedMacs { > Inner types usually go at end of class. Done Line 363: void removeMacFromNotUsed(int macIndex); Line 364: Line 365: void addMacAmongFree(int macIndex); Line 366: Line 358: private int _notUsedCount; Line 359: Line 360: private NotUsedMacs notUsedMacs; Line 361: Line 362: public interface NotUsedMacs { > Why to be so negative? :-) Done Line 363: void removeMacFromNotUsed(int macIndex); Line 364: Line 365: void addMacAmongFree(int macIndex); Line 366: Line 366: Line 367: long unusedMac(long offset); Line 368: } Line 369: Line 370: private static class NotUsedMacsBitSet implements NotUsedMacs { > the class independence + test goes here too Done Line 371: Line 372: private BitSet bitSet; Line 373: Line 374: public NotUsedMacsBitSet(int size) { Line 371: Line 372: private BitSet bitSet; Line 373: Line 374: public NotUsedMacsBitSet(int size) { Line 375: this.bitSet = new BitSet(size); > Why address with "this"? same Line 376: bitSet.set(0, size, true); Line 377: } Line 378: Line 379: @Override Line 377: } Line 378: Line 379: @Override Line 380: public void removeMacFromNotUsed(int macIndex) { Line 381: this.bitSet.set(macIndex, false); > Why address with "this"? same Line 382: } Line 383: Line 384: @Override Line 385: public void addMacAmongFree(int macIndex) { Line 382: } Line 383: Line 384: @Override Line 385: public void addMacAmongFree(int macIndex) { Line 386: this.bitSet.set(macIndex, true); > Why address with "this"? same Line 387: } Line 388: Line 389: @Override Line 390: public long unusedMac(long offset) { Line 390: public long unusedMac(long offset) { Line 391: int start = 0; Line 392: int notUsedMacIndex = 0; Line 393: while (notUsedMacIndex != -1) { Line 394: notUsedMacIndex = this.bitSet.nextSetBit(start); > Why address with "this"? same Line 395: start = notUsedMacIndex; Line 396: long mac = offset + notUsedMacIndex; Line 397: if (MacAddressRangeUtils.macHasMultiCastBitSet(mac)) { Line 398: removeMacFromNotUsed(notUsedMacIndex); Line 407: Line 408: public Range(long offset, int size, int numberOfNonMultiCasts) { Line 409: this.offset = offset; Line 410: this.size = size; Line 411: this._notUsedCount = numberOfNonMultiCasts; > What is this _ syntax? Done Line 412: Line 413: macUsageCount = new int[size]; Line 414: this.notUsedMacs = new NotUsedMacsBitSet(size); Line 415: } Line 410: this.size = size; Line 411: this._notUsedCount = numberOfNonMultiCasts; Line 412: Line 413: macUsageCount = new int[size]; Line 414: this.notUsedMacs = new NotUsedMacsBitSet(size); > Why address with "this"? same Line 415: } Line 416: Line 417: /** Line 418: * Line 426: int currentCount = test(index); Line 427: Line 428: if (currentCount == 0) { Line 429: _notUsedCount --; Line 430: this.notUsedMacs.removeMacFromNotUsed(macToArrayIndex(mac)); > Why address with "this"? same Line 431: macUsageCount[index]+=1; Line 432: return true; Line 433: } Line 434: Line 443: } Line 444: Line 445: } Line 446: Line 447: int test(int index) { > 1. please make it private Done Line 448: return macUsageCount[index]; Line 449: } Line 450: Line 451: int macToArrayIndex(long mac) { Line 447: int test(int index) { Line 448: return macUsageCount[index]; Line 449: } Line 450: Line 451: int macToArrayIndex(long mac) { > please make it private Done Line 452: return (int) (mac - offset); Line 453: } Line 454: Line 455: public boolean isAllocated(long mac) { Line 466: } else { Line 467: macUsageCount[index] -= 1; Line 468: Line 469: if (currentCount == 1) { //was there and now it's gone. It's free.\ Line 470: this.notUsedMacs.addMacAmongFree(macToArrayIndex(mac)); > Why address with "this"? same Line 471: _notUsedCount++; Line 472: return true; Line 473: } else { Line 474: return false; Line 476: Line 477: } Line 478: } Line 479: Line 480: public int getNotUsedCount() { > IMHO getAvailableCount is better name Done Line 481: return _notUsedCount; Line 482: } Line 483: Line 484: public long[] allocateMac(int numberOfMacs) { http://gerrit.ovirt.org/#/c/26405/6/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/MacPoolManagerInitializationTimeTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/MacPoolManagerInitializationTimeTest.java: Line 4: import org.junit.Test; Line 5: Line 6: //add cmd-line argument before run: " -javaagent:${HOME}/.m2/repository/com/github/stephenc/jamm/0.2.5/jamm-0.2.5.jar " Line 7: Line 8: @Ignore("Maven is not configured to run this automatically, you have to run it manually.") > This actually not a unit test but a performance test. Do we have to keep in I think performance tests are still unit tests. It's pretty valid to have unit tests testing code response time or memory complexity. This code was primarily used for measurement during development and was pushed to demonstrate improvement to RFE owner. I'm removing it now. Line 9: public class MacPoolManagerInitializationTimeTest { Line 10: Line 11: public static final int MAX_MACS_IN_POOL = 1000000; Line 12: // public static final int MAX_MACS_IN_POOL = 100000; http://gerrit.ovirt.org/#/c/26405/6/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/MacPoolManagerValidityTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/MacPoolManagerValidityTest.java: Line 19: import org.ovirt.engine.core.common.errors.VdcBllErrors; Line 20: import org.ovirt.engine.core.utils.MacAddressRangeUtils; Line 21: Line 22: @Ignore Line 23: public class MacPoolManagerValidityTest { > This actually not a unit test but a performance test. Do we have to keep in like I said in another comment — performance test is still unit test. This is not only performance test, but you'll find contract checking tests as well. This test was pushed to demostrate improvement in implementation and same functionality as original implementation has. I'm removing this test as well as original implementation which is rendered useless by this removal (and one similar one). Line 24: Line 25: //disable mockito when testing on large ranges -- it slows process down, can even crash. Line 26: public static final boolean DO_VERIFICATIONS_USING_MOCKITO = false; Line 27: public static final int MAX_MACS_IN_POOL = 1000000; -- To view, visit http://gerrit.ovirt.org/26405 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I69a5c1b3b43966e49fa6039597c06966ce514618 Gerrit-PatchSet: 6 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: Yevgeny Zaspitsky <[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
