Mike Kolesnik has posted comments on this change. Change subject: core: MacPoolManager strategy, alternative implementation, test for benchmarks ......................................................................
Patch Set 6: (29 comments) I stopped reviewing in the middle because the flow was getting hard to follow with the various nested classes. Please address the comments and split into separate classes which would be easier to review. 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 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"? Line 46: Line 47: Line 48: List<VmNic> interfaces = getVmNicInterfacesFromDB(); Line 49: Line 50: for (VmNic iface : interfaces) { Line 51: forceAddMac(iface.getMacAddress()); Line 52: } Line 53: initialized = true; Line 54: log.infoFormat("Finished initializing. Available MACs in pool: {0}", this.macsStorage.getAvailableMacsCount()); Why address with "this"? Line 55: } catch (Exception ex) { Line 56: log.errorFormat("Error in initializing MAC Addresses pool manager.", ex); Line 57: } finally { Line 58: lockObj.writeLock().unlock(); Line 61: Line 62: MacsStorage initRanges(String rangesString) { Line 63: List<Pair<Long, Long>> rangesBoundaries = MacAddressRangeUtils.rangesStringToRangeBoundaries(rangesString); Line 64: int macsToCreate = maxMacsInPool; Line 65: MacsStorage macsStorage = new MacsStorage(this.allowDuplicates); Why address with "this"? Line 66: for (Pair<Long, Long> rangesBoundary : rangesBoundaries) { Line 67: macsToCreate -= macsStorage.addRange(rangesBoundary.getFirst(), rangesBoundary.getSecond(), macsToCreate); Line 68: } Line 69: 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 Or you could argue that it belongs in MacPoolManager, regardless of which strategy you choose to use (assuming you have more than one, of course). 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. Why not plural method name? 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 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"? Line 165: if (macsStorage.noAvailableMacs()) { Line 166: logMacPoolEmpty(); Line 167: } Line 168: return added; 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"? 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"? 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? 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? Line 212: for (String mac : macs) { Line 213: macsStorage.freeMac(MacAddressRangeUtils.macStringToLong(mac)); Line 214: } Line 215: 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"? 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 Agreed 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? 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 Line 251: Line 252: long i = first; Line 253: int numberOfNonMultiCasts = 0; Line 254: Line 252: long i = first; Line 253: int numberOfNonMultiCasts = 0; Line 254: Line 255: for (; i < second && numberOfNonMultiCasts < maxMacsInPool; i++) { Line 256: if ((i & MacAddressRangeUtils.MAC_ADDRESS_MULTICAST_BIT) == 0) { //not a multicast Why not use the extracted logic that tests if multicast? Also this can be improved if you notice that the range can be split up to not contain any multicast addresses.. Say you have range 00:00:00:00:00:00-02:ff:ff:ff:ff:ff then all macs starting with 01 are multicast versions of all macs starting with 00, so the range can be split into the ranges: 00:00:00:00:00:00-00:ff:ff:ff:ff:ff,02:00:00:00:00:00-02:ff:ff:ff:ff:ff and you don't need to iterate over 2^62 mac addresses. This improvement can (and probably should) take place, however, at a later patch. Line 257: numberOfNonMultiCasts++; Line 258: } Line 259: } Line 260: 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 original implementation.. Maybe there's a way to overcome this..? Perhaps a similar method to http://java-performance.info/bit-sets/ 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"? 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); Why address with "this"? Line 296: } Line 297: Line 298: public boolean freeMac(long mac) { Line 299: testForMultiCastMac(mac); 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"? Line 306: } Line 307: } Line 308: Line 309: public boolean noAvailableMacs() { 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 Second that Line 348: return range; Line 349: } Line 350: } Line 351: return null; 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. Line 363: void removeMacFromNotUsed(int macIndex); Line 364: Line 365: void addMacAmongFree(int macIndex); Line 366: 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"? 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"? 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"? 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"? 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? 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"? 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"? Line 431: macUsageCount[index]+=1; Line 432: return true; Line 433: } Line 434: 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"? Line 471: _notUsedCount++; Line 472: return true; Line 473: } else { Line 474: return false; -- 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
