Mike Kolesnik has posted comments on this change. Change subject: core: MacPoolManager strategy, alternative implementation, test for benchmarks ......................................................................
Patch Set 18: (62 comments) http://gerrit.ovirt.org/#/c/26405/18//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2014-04-03 15:34:31 +0200 Line 4: Commit: Martin Mucha <mmu...@redhat.com> Line 5: CommitDate: 2014-05-16 08:40:03 +0200 Line 6: Line 7: core: MacPoolManager strategy, alternative implementation, test for Pretty sure this needs to be updated. Line 8: benchmarks Line 9: Line 10: * internals of MacPoolManager moved to separate implementation of Line 11: MacPoolManagerStrategy (MacPoolManagerStrategyOriginal) to allow Line 31: *MacPoolManager is singleton; only 'getInstance()' has to be Line 32: static other methods should be instance methods. Having them Line 33: static is mixing concepts. Line 34: --- Line 35: space costs(for 1 000 000 MACs range): All the information bought forth is nice and very informative, but it would be better placed in the wiki or as a blog post. You could also post there your benchmarking code with instructions of how to run it. Line 36: (these measurements need not be accurate, but it's probably the Line 37: best you can get from JVM): Line 38: Measurement for: MacPoolManagerOriginal------ Line 39: Time: 794 ms http://gerrit.ovirt.org/#/c/26405/18/backend/manager/modules/bll/pom.xml File backend/manager/modules/bll/pom.xml: Line 18: <engine.schema>engine</engine.schema> Line 19: </properties> Line 20: Line 21: <dependencies> Line 22: Not necessary to touch pom.xml at all, right? Line 23: <dependency> Line 24: <groupId>${engine.groupId}</groupId> Line 25: <artifactId>compat</artifactId> Line 26: <version>${engine.version}</version> http://gerrit.ovirt.org/#/c/26405/18/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacPoolManagerOriginal.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacPoolManagerOriginal.java: Line 21 Line 22 Line 23 Line 24 Line 25 Due to this change, org.ovirt.engine.core.utils.MacAddressRangeUtils.initRange(String, String, int) method can be safetly deleted and the other methods adjusted.. I suggest you do it in separate patch which fixes EngineCofig tool as well (or seeing as you don't plan to use the config value anymore, that class can be mostly deleted after not using the config value). http://gerrit.ovirt.org/#/c/26405/18/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 19: public class MacPoolManagerRanges implements MacPoolManagerStrategy{ Line 20: Line 21: private static final Log log = LogFactory.getLog(MacPoolManagerRanges.class); Line 22: Line 23: private final int maxMacsInPool; This should be deleted since there will be no possibility of memory leak, and this value was just a limit to prevent memory leaks.. Line 24: private final String rangesString; Line 25: Line 26: private final ReentrantReadWriteLock lockObj = new ReentrantReadWriteLock(); Line 27: private final Boolean allowDuplicates; Line 23: private final int maxMacsInPool; Line 24: private final String rangesString; Line 25: Line 26: private final ReentrantReadWriteLock lockObj = new ReentrantReadWriteLock(); Line 27: private final Boolean allowDuplicates; Would null value make sense? Line 28: private boolean initialized; Line 29: private MacsStorage macsStorage; Line 30: Line 31: public MacPoolManagerRanges(Integer maxMacsInPool, String rangesString, Boolean allowDuplicates) { Line 35: } Line 36: Line 37: @Override Line 38: public void initialize() { Line 39: checkNotInitialized(); This isn't thread safe. Also MacPoolManagerOriginal had no problem being initialized more than once, so don't change the behavior. Line 40: Line 41: lockObj.writeLock().lock(); Line 42: try { Line 43: log.infoFormat("Start initializing "+getClass().getSimpleName()); Line 40: Line 41: lockObj.writeLock().lock(); Line 42: try { Line 43: log.infoFormat("Start initializing "+getClass().getSimpleName()); Line 44: Not sure why there are so many empty lines between some code, and no new line after closing bracket.. Line 45: this.macsStorage = createMacsStorage(rangesString); Line 46: Line 47: Line 48: List<VmNic> interfaces = getVmNicInterfacesFromDB(); Line 41: lockObj.writeLock().lock(); Line 42: try { Line 43: log.infoFormat("Start initializing "+getClass().getSimpleName()); Line 44: Line 45: this.macsStorage = createMacsStorage(rangesString); Don't call with this Line 46: Line 47: Line 48: List<VmNic> interfaces = getVmNicInterfacesFromDB(); Line 49: Line 50: for (VmNic iface : interfaces) { Line 51: forceAddMacWithoutLocking(iface.getMacAddress()); Line 52: } Line 53: initialized = true; Line 54: log.infoFormat("Finished initializing. Available MACs in pool: {0}", this.macsStorage.getAvailableMacsCount()); Don't call 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: private MacsStorage createMacsStorage(String rangesString) { Line 63: List<Pair<Long, Long>> rangesBoundaries = MacAddressRangeUtils.rangesTextualDefinitionToRangeBoundaries(rangesString); Line 64: int macsToCreate = maxMacsInPool; Line 65: MacsStorage macsStorage = new MacsStorage(this.allowDuplicates); Don't call 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 62: private MacsStorage createMacsStorage(String rangesString) { Line 63: List<Pair<Long, Long>> rangesBoundaries = MacAddressRangeUtils.rangesTextualDefinitionToRangeBoundaries(rangesString); Line 64: int macsToCreate = maxMacsInPool; Line 65: MacsStorage macsStorage = new MacsStorage(this.allowDuplicates); Line 66: for (Pair<Long, Long> rangesBoundary : rangesBoundaries) { Shouldn't the single instance be called just range? Line 67: macsToCreate -= macsStorage.addRange(rangesBoundary.getFirst(), rangesBoundary.getSecond(), macsToCreate); Line 68: } Line 69: Line 70: if (macsStorage.noAvailableMacs()) { Line 68: } Line 69: Line 70: if (macsStorage.noAvailableMacs()) { Line 71: throw new VdcBLLException(VdcBllErrors.MAC_POOL_INITIALIZATION_FAILED); Line 72: } else { No need for else clause here Line 73: return macsStorage; Line 74: } Line 75: } Line 76: Line 73: return macsStorage; Line 74: } Line 75: } Line 76: Line 77: private List<VmNic> getVmNicInterfacesFromDB() { s/DB/Db/ Line 78: return DbFacade.getInstance().getVmNicDao().getAll(); Line 79: } Line 80: Line 81: private void logMacPoolEmpty() { Line 77: private List<VmNic> getVmNicInterfacesFromDB() { Line 78: return DbFacade.getInstance().getVmNicDao().getAll(); Line 79: } Line 80: Line 81: private void logMacPoolEmpty() { I would put the "if (macsStorage.noAvailableMacs()) {" inside, since it's plastered all over the code and no need to really copy it. Method name can be logWhenMacPoolEmpty Line 82: AuditLogableBase logable = new AuditLogableBase(); Line 83: AuditLogDirector.log(logable, AuditLogType.MAC_POOL_EMPTY); Line 84: } Line 85: Line 84: } Line 85: Line 86: @Override Line 87: public String allocateNewMac() { Line 88: checkInitialized(); This isn't thread safe Line 89: lockObj.writeLock().lock(); Line 90: try { Line 91: return allocateNewMacsWithoutLocking(1).get(0); Line 92: } finally { Line 94: } Line 95: } Line 96: Line 97: private List<String> allocateNewMacsWithoutLocking(int numberOfMacs) { Line 98: if (!initialized) { Why do you need this if you already checked if initialized in both places calling this method? Line 99: logInitializationError("Failed to allocate new Mac address."); Line 100: throw new VdcBLLException(VdcBllErrors.MAC_POOL_NOT_INITIALIZED); Line 101: } Line 102: Line 110: } Line 111: Line 112: @Override Line 113: public int getAvailableMacsCount() { Line 114: checkInitialized(); This isn't thread safe Line 115: lockObj.readLock().lock(); Line 116: try { Line 117: if (!initialized) { Line 118: logInitializationError("Failed to get available Macs count."); Line 113: public int getAvailableMacsCount() { Line 114: checkInitialized(); Line 115: lockObj.readLock().lock(); Line 116: try { Line 117: if (!initialized) { Why check again if initialized? Line 118: logInitializationError("Failed to get available Macs count."); Line 119: throw new VdcBLLException(VdcBllErrors.MAC_POOL_NOT_INITIALIZED); Line 120: } Line 121: Line 128: } Line 129: Line 130: @Override Line 131: public void freeMac(String mac) { Line 132: checkInitialized(); This isn't thread safe Line 133: lockObj.writeLock().lock(); Line 134: try { Line 135: if (!initialized) { Line 136: logInitializationError("Failed to free mac address " + mac + " ."); Line 131: public void freeMac(String mac) { Line 132: checkInitialized(); Line 133: lockObj.writeLock().lock(); Line 134: try { Line 135: if (!initialized) { Why check again if initialized? Line 136: logInitializationError("Failed to free mac address " + mac + " ."); Line 137: } else { Line 138: macsStorage.freeMac(MacAddressRangeUtils.macToLong(mac)); Line 139: } Line 150: } Line 151: Line 152: @Override Line 153: public boolean addMac(String mac) { Line 154: checkInitialized(); This isn't thread safe Line 155: lockObj.writeLock().lock(); Line 156: try { Line 157: boolean added = this.macsStorage.useMac(MacAddressRangeUtils.macToLong(mac)); Line 158: if (macsStorage.noAvailableMacs()) { Line 153: public boolean addMac(String mac) { Line 154: checkInitialized(); Line 155: lockObj.writeLock().lock(); Line 156: try { Line 157: boolean added = this.macsStorage.useMac(MacAddressRangeUtils.macToLong(mac)); Don't call with this Line 158: if (macsStorage.noAvailableMacs()) { Line 159: logMacPoolEmpty(); Line 160: } Line 161: return added; Line 167: } Line 168: Line 169: @Override Line 170: public void forceAddMac(String mac) { Line 171: checkInitialized(); This isn't thread safe Line 172: lockObj.writeLock().lock(); Line 173: try { Line 174: this.macsStorage.useMacNoDuplicityCheck(MacAddressRangeUtils.macToLong(mac)); Line 175: if (macsStorage.noAvailableMacs()) { Line 170: public void forceAddMac(String mac) { Line 171: checkInitialized(); Line 172: lockObj.writeLock().lock(); Line 173: try { Line 174: this.macsStorage.useMacNoDuplicityCheck(MacAddressRangeUtils.macToLong(mac)); Why not call forceAddMacWithoutLocking? Line 175: if (macsStorage.noAvailableMacs()) { Line 176: logMacPoolEmpty(); Line 177: } Line 178: } finally { Line 180: } Line 181: } Line 182: Line 183: private void forceAddMacWithoutLocking(String mac) { Line 184: this.macsStorage.useMacNoDuplicityCheck(MacAddressRangeUtils.macToLong(mac)); Don't call with this Line 185: if (macsStorage.noAvailableMacs()) { Line 186: logMacPoolEmpty(); Line 187: } Line 188: } Line 188: } Line 189: Line 190: @Override Line 191: public boolean isMacInUse(String mac) { Line 192: checkInitialized(); This isn't thread safe Line 193: lockObj.readLock().lock(); Line 194: try { Line 195: return this.macsStorage.isMacInUse(MacAddressRangeUtils.macToLong(mac)); Line 196: Line 191: public boolean isMacInUse(String mac) { Line 192: checkInitialized(); Line 193: lockObj.readLock().lock(); Line 194: try { Line 195: return this.macsStorage.isMacInUse(MacAddressRangeUtils.macToLong(mac)); Don't call with this Line 196: Line 197: } finally { Line 198: lockObj.readLock().unlock(); Line 199: } Line 199: } Line 200: } Line 201: Line 202: @Override Line 203: public void freeMacs(List<String> macs) { //TODO MM: how about some duplicities here? Release it twice or what? What did original code do? Line 204: checkInitialized(); Line 205: if (!macs.isEmpty()) { Line 206: lockObj.writeLock().lock(); Line 207: try { Line 200: } Line 201: Line 202: @Override Line 203: public void freeMacs(List<String> macs) { //TODO MM: how about some duplicities here? Release it twice or what? Line 204: checkInitialized(); This isn't thread safe Line 205: if (!macs.isEmpty()) { Line 206: lockObj.writeLock().lock(); Line 207: try { Line 208: for (String mac : macs) { Line 216: } Line 217: Line 218: @Override Line 219: public List<String> allocateMacAddresses(int numberOfAddresses) { Line 220: checkInitialized(); This isn't thread safe Line 221: lockObj.writeLock().lock(); Line 222: try { Line 223: List<String> macAddresses = this.allocateNewMacsWithoutLocking(numberOfAddresses); Line 224: return macAddresses; Line 219: public List<String> allocateMacAddresses(int numberOfAddresses) { Line 220: checkInitialized(); Line 221: lockObj.writeLock().lock(); Line 222: try { Line 223: List<String> macAddresses = this.allocateNewMacsWithoutLocking(numberOfAddresses); Don't call with this Line 224: return macAddresses; Line 225: } finally { Line 226: lockObj.writeLock().unlock(); Line 227: } Line 220: checkInitialized(); Line 221: lockObj.writeLock().lock(); Line 222: try { Line 223: List<String> macAddresses = this.allocateNewMacsWithoutLocking(numberOfAddresses); Line 224: return macAddresses; You can return immediately, no need for variable Line 225: } finally { Line 226: lockObj.writeLock().unlock(); Line 227: } Line 228: } Line 233: throw new IllegalStateException("This instance was already initialized."); Line 234: } Line 235: } Line 236: Line 237: private void checkInitialized() { You should call logInitializationError and also throw a VdcBLLException(VdcBllErrors.MAC_POOL_NOT_INITIALIZED) which provides a clear message to the user. Also you shouldn't call this in places that didn't do it before as you're breaking the API of MacPoolManager You can add the call in later commits if it makes sense, but since this is about memory optimization and not initialization checks, I don't see how this patch should change the existing API. Line 238: if (!initialized) { Line 239: throw new IllegalStateException("This instance not yet initialized."); Line 240: } Line 241: Line 241: Line 242: } Line 243: Line 244: @Override Line 245: public void copyContent(MacPoolManagerStrategy macPoolManagerStrategy) { This method is unused.. Please remove and add when used Line 246: macsStorage.copyAllocatedCustomMacs(macPoolManagerStrategy); Line 247: macsStorage.copyAllocatedMacs(macPoolManagerStrategy); Line 248: } Line 249: http://gerrit.ovirt.org/#/c/26405/18/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacPoolManagerStrategy.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacPoolManagerStrategy.java: Line 59: * @throws org.ovirt.engine.core.common.errors.VdcBLLException if mac address cannot be allocated. Line 60: */ Line 61: List<String> allocateMacAddresses(int numberOfAddresses); Line 62: Line 63: void copyContent(MacPoolManagerStrategy macPoolManagerStrategy); Why add unused method? Line 64: Line 65: http://gerrit.ovirt.org/#/c/26405/18/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacsStorage.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacsStorage.java: Line 28: } Line 29: } Line 30: Line 31: long size = i - rangeStart; Line 32: if (size > Integer.MAX_VALUE) { If you're already limiting to Integer.MAX_VALUE, there is no sense to look for multicast MACs by iteration. Since MAC is composed of 48 bits, and the multicast bit is no. 40 (LSB being 0), there isn't a way to specify a range that has an "area" of multicast MACs. The range could have one of four possible situations: 1. No multicast MACs. 2. Only multicast MACs in the beginning of the range. 3. Only multicast MACs in the end of the range. 4. All multicast MACs. So handling multicast MACs given the size limitation of each range should be easy - trim multicast start & end. Basically something like: if (MacAddressRangeUtils.macIsMulticast(rangeStart)) { rangeStart = (rangeStart | 0x00FFFFFFFFFFL) + 1; } if (MacAddressRangeUtils.macIsMulticast(rangeEnd)) { rangeEnd = (rangeEnd & 0xFF0000000000L) - 1; } if (rangeStart > rangeEnd) { throw new IllegalArgumentException("Can't have only multicast range."); } Line 33: throw new IllegalArgumentException("Not supported that wide ranges(" + size + ")."); Line 34: } Line 35: Line 36: if (numberOfNonMultiCasts > 0) { Line 29: } Line 30: Line 31: long size = i - rangeStart; Line 32: if (size > Integer.MAX_VALUE) { Line 33: throw new IllegalArgumentException("Not supported that wide ranges(" + size + ")."); Since you decided to limit this, please take care of upgrade so that if the user specified a bigger range his system will fail to work, otherwise you cause a regression of the current system behavior. Line 34: } Line 35: Line 36: if (numberOfNonMultiCasts > 0) { Line 37: //java. arrays/collections can not be bigger than 2^32. Line 33: throw new IllegalArgumentException("Not supported that wide ranges(" + size + ")."); Line 34: } Line 35: Line 36: if (numberOfNonMultiCasts > 0) { Line 37: //java. arrays/collections can not be bigger than 2^32. Not sure why this comment is relevant Line 38: ranges.add(new Range(rangeStart, (int)size, numberOfNonMultiCasts)); Line 39: } Line 40: return numberOfNonMultiCasts; Line 41: } Line 43: public boolean useMac(long mac) { Line 44: return useMac(mac, allowDuplicates); Line 45: } Line 46: Line 47: private boolean useMac(long mac, Boolean allowDuplicates) { What is the significance of a null allowDuplicates? Line 48: testForMultiCastMac(mac); Line 49: Line 50: Range range = findIncludingRange(mac); Line 51: if (range == null) { Line 48: testForMultiCastMac(mac); Line 49: Line 50: Range range = findIncludingRange(mac); Line 51: if (range == null) { Line 52: return this.customMacs.add(mac); Don't use this Also, this is not considering the sent value of allowDuplicates, which is very problematic. Line 53: } else { Line 54: return range.use(mac, allowDuplicates); Line 55: } Line 56: } Line 62: public boolean isMacInUse(long mac) { Line 63: testForMultiCastMac(mac); Line 64: Line 65: Range range = findIncludingRange(mac); Line 66: return range == null ? this.customMacs.contains(mac) : range.isAllocated(mac); Don't use this Line 67: } Line 68: Line 69: public void freeMac(long mac) { Line 70: testForMultiCastMac(mac); Line 70: testForMultiCastMac(mac); Line 71: Line 72: Range range = findIncludingRange(mac); Line 73: if (range == null) { Line 74: this.customMacs.remove(mac); Don't use this Line 75: } else { Line 76: range.freeMac(mac); Line 77: } Line 78: } Line 76: range.freeMac(mac); Line 77: } Line 78: } Line 79: Line 80: public boolean noAvailableMacs() { Please change to positive method, as double negative is harder to maintain Line 81: for (Range range : ranges) { Line 82: if (range.getAvailableCount() > 0) { Line 83: return false; Line 84: } Line 85: } Line 86: return true; Line 87: } Line 88: Line 89: public List<Long> allocateAvailableMac(int numberOfMacs) { Should be allocateAvailableMacs Line 90: if (getAvailableMacsCount() < numberOfMacs) { //TODO MM: allocate just those remaining or none? Line 91: throw new VdcBLLException(VdcBllErrors.MAC_POOL_NO_MACS_LEFT); Line 92: } Line 93: Line 86: return true; Line 87: } Line 88: Line 89: public List<Long> allocateAvailableMac(int numberOfMacs) { Line 90: if (getAvailableMacsCount() < numberOfMacs) { //TODO MM: allocate just those remaining or none? Don't allocate any MACs if there aren't enough Line 91: throw new VdcBLLException(VdcBllErrors.MAC_POOL_NO_MACS_LEFT); Line 92: } Line 93: Line 94: return getRangeWithAvailableMac().allocateMac(numberOfMacs); Line 90: if (getAvailableMacsCount() < numberOfMacs) { //TODO MM: allocate just those remaining or none? Line 91: throw new VdcBLLException(VdcBllErrors.MAC_POOL_NO_MACS_LEFT); Line 92: } Line 93: Line 94: return getRangeWithAvailableMac().allocateMac(numberOfMacs); What if there is no one range with the free amount of MACs, but across several ranges it's possible? Line 95: } Line 96: Line 97: private Range getRangeWithAvailableMac() { Line 98: for (Range range : ranges) { Line 110: } Line 111: return count; Line 112: } Line 113: Line 114: private void testForMultiCastMac(long mac) { No need for this method Line 115: if (MacAddressRangeUtils.macIsMulticast(mac)) { Line 116: throw new IllegalArgumentException("You're trying to work with multicast address."); //TODO MM: should this be prohibited? Line 117: } Line 118: } Line 125: } Line 126: return null; Line 127: } Line 128: Line 129: public void copyAllocatedCustomMacs(MacPoolManagerStrategy macPoolManagerStrategy) { Please remove and add when used Line 130: for (Long mac : customMacs) { Line 131: final int count = customMacs.count(mac); Line 132: final String macString = MacAddressRangeUtils.macToString(mac); Line 133: Line 136: } Line 137: } Line 138: } Line 139: Line 140: public void copyAllocatedMacs(MacPoolManagerStrategy macPoolManagerStrategy) { Please remove and add when used Line 141: for (Range range : ranges) { Line 142: range.copyAllocatedMacs(macPoolManagerStrategy); Line 143: } Line 144: } http://gerrit.ovirt.org/#/c/26405/18/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/Range.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/Range.java: Line 12: private int notUsedCount; Line 13: Line 14: private UsedMacs usedMacs; Line 15: Line 16: public boolean contains(long mac) { Why is this before the c'tor? Line 17: return offset <= mac && (offset + size) >= mac; Line 18: } Line 19: Line 20: public Range(long offset, int size, int numberOfNonMultiCasts) { Line 21: this.offset = offset; Line 22: this.size = size; Line 23: this.notUsedCount = numberOfNonMultiCasts; Line 24: Line 25: macUsageCount = new int[size]; This seems very problematic to me, if user defines a range of same OUI (i.e. 00:00:00:00:00:00-00:00:00:FF:FF:FF) it would take 4*2^24 bytes = 64Mb, Even if no MACs from the range are consumed, it would take that amount. Line 26: this.usedMacs = new UsedMacsBitSet(size); Line 27: } Line 28: Line 29: /** Line 29: /** Line 30: * Line 31: * @param mac mac to add Line 32: * @return if mac was used (it's usage count was increased). I.e. if it was not used, it's used now, or Line 33: * it was used and duplicates are allowed so it's not used some more. "so it's not used some more"? Line 34: */ Line 35: public boolean use(long mac, boolean allowDuplicates) { Line 36: try { Line 37: int index = macToArrayIndex(mac); Line 34: */ Line 35: public boolean use(long mac, boolean allowDuplicates) { Line 36: try { Line 37: int index = macToArrayIndex(mac); Line 38: int currentCount = currentCount(index); Why assign to variable that is used only once? Line 39: Line 40: if (currentCount == 0) { Line 41: notUsedCount--; Line 42: this.usedMacs.removeMacFromNotUsed(macToArrayIndex(mac)); Line 49: return true; Line 50: } Line 51: Line 52: return false; Line 53: } catch (Exception e) { What exception do you expect to happen? Line 54: throw new RuntimeException("failed use during adding mac: " + mac + ", index = " + (mac - offset), e); Line 55: } Line 56: Line 57: } Line 55: } Line 56: Line 57: } Line 58: Line 59: private int currentCount(int index) { what is the necessity of this method? Line 60: return macUsageCount[index]; Line 61: } Line 62: Line 63: private int macToArrayIndex(long mac) { Line 65: } Line 66: Line 67: public boolean isAllocated(long mac) { Line 68: int index = macToArrayIndex(mac); Line 69: int currentCount = macUsageCount[index]; This can all be inlined Line 70: return currentCount > 0; Line 71: } Line 72: Line 73: public void freeMac(long mac) { Line 70: return currentCount > 0; Line 71: } Line 72: Line 73: public void freeMac(long mac) { Line 74: int index = macToArrayIndex(mac); This can be inlined Line 75: int currentCount = macUsageCount[index]; Line 76: if (currentCount == 0) { Line 77: return; Line 78: } Line 89: public int getAvailableCount() { Line 90: return notUsedCount; Line 91: } Line 92: Line 93: public List<Long> allocateMac(int numberOfMacs) { Should be called allocateMacs Line 94: List<Long> result = new ArrayList<>(numberOfMacs); Line 95: Line 96: int count = 0; Line 97: while (count < numberOfMacs) { Line 93: public List<Long> allocateMac(int numberOfMacs) { Line 94: List<Long> result = new ArrayList<>(numberOfMacs); Line 95: Line 96: int count = 0; Line 97: while (count < numberOfMacs) { Why not for loop? Line 98: final long mac = usedMacs.unusedMac(offset); Line 99: use(mac, false); //well duplicates may be allowed, but we're using unallocated mac. Line 100: count++; Line 101: result.add(mac); Line 95: Line 96: int count = 0; Line 97: while (count < numberOfMacs) { Line 98: final long mac = usedMacs.unusedMac(offset); Line 99: use(mac, false); //well duplicates may be allowed, but we're using unallocated mac. Please write comment before code. Line 100: count++; Line 101: result.add(mac); Line 102: } Line 103: http://gerrit.ovirt.org/#/c/26405/18/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/UsedMacsBitSet.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/UsedMacsBitSet.java: Line 3: import java.util.BitSet; Line 4: Line 5: import org.ovirt.engine.core.utils.MacAddressRangeUtils; Line 6: Line 7: class UsedMacsBitSet implements UsedMacs { This class is unnecessary and confusing, please remove it Line 8: Line 9: private BitSet bitSet; Line 10: Line 11: public UsedMacsBitSet(int size) { -- 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: 18 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
