Yevgeny Zaspitsky has posted comments on this change. Change subject: core: MacPoolManager strategy, alternative implementation, test for benchmarks ......................................................................
Patch Set 6: (32 comments) 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? 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 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? Line 41: lockObj.writeLock().lock(); Line 42: try { Line 43: log.infoFormat("Start initializing "+getClass().getSimpleName()); Line 44: Line 58: lockObj.writeLock().unlock(); Line 59: } Line 60: } Line 61: Line 62: MacsStorage initRanges(String rangesString) { 1. please make it private 2. createMacStorage would be a beter name 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 the DAO in the unit test and verify the class interaction with the DAO. 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 functionality which is static in AuditLogDirector. I'd prefer passing the Delegator instance through the constructor, which will be a good preparation for DI. (you have to rebase on updated master branch in order to find AuditLogDirectorDelegator) 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(); IMHO thread safeness mechanism should be moved to MacStorage class. Once it's instantiated it should be able to maintaine clean and tread safe interface. 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. 2. Please find a better method name. Usually "Impl" is given to as a suffix to an implementation class. 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 caller methods already 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 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 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 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 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 Line 178: * Line 179: */ Line 180: @Override Line 181: public void forceAddMac(String mac) { 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 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 227: * @return The list of MAC addresses, sorted in ascending order Line 228: */ Line 229: @Override Line 230: public List<String> allocateMacAddresses(int numberOfAddresses) { Line 231: lockObj.writeLock().lock(); please move the locks to MacStorage class Line 232: try { Line 233: String[] macAddresses = this.allocateNewMacImpl(numberOfAddresses); Line 234: return Arrays.asList(macAddresses); Line 235: } finally { 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 class (not to mention its inner classes) and to have its own test. If you bother about its visibility outside (you might argue that's the inner implementation) all those inner classes and interfaces could become package protected. Line 241: private final Boolean allowDuplicates; Line 242: private List<Range> ranges = new LinkedList<>(); Line 243: private ObjectCounter<Long> customMacs; Line 244: 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 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 Line 303: return range.freeMac(mac); Line 304: } else { Line 305: return this.customMacs.remove(mac); Line 306: } 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 available and numberOfMacs is 2? 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 IllegalArgumentException would be translated into HTTP 500 Line 342: } Line 343: } Line 344: Line 345: private Range getRange(long mac) { Line 345: getRange findIncludingRange is more descriptive here 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 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 2. I'd split The class into 2 implementations (allowing and disallowing duplicates) 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? 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 "_" 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 { Why to be so negative? :-) IMHO FreeMacsBitMap or FreeMacPool are better names 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 Line 371: Line 372: private BitSet bitSet; Line 373: Line 374: public NotUsedMacsBitSet(int size) { Line 443: } Line 444: Line 445: } Line 446: Line 447: int test(int index) { 1. please make it private 2. please choose a more descriptive method name 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 Line 452: return (int) (mac - offset); Line 453: } Line 454: Line 455: public boolean isAllocated(long mac) { Line 476: Line 477: } Line 478: } Line 479: Line 480: public int getNotUsedCount() { IMHO getAvailableCount is better name 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 Git if we ignore it? 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 Git if we ignore it? 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
