Mike Kolesnik has posted comments on this change. Change subject: core: MacPoolManager strategy, alternative implementation, test for benchmarks ......................................................................
Patch Set 6: (5 comments) 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{ > http://www.ovirt.org/Building_Ovirt_Engine/IDE As Yevgeny stated, please use the formatter. Line 22: Line 23: private static final Log log = LogFactory.getLog(MacPoolManagerRanges.class); Line 24: Line 25: private final int maxMacsInPool; 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 not? Does it worsen code base or emphasize that this is field not local We don't use it in the project and you don't use it consistently yourself.. This is Java not Python. Line 46: Line 47: Line 48: List<VmNic> interfaces = getVmNicInterfacesFromDB(); Line 49: Line 86: } Line 87: Line 88: @Override Line 89: public String allocateNewMac() { Line 90: lockObj.writeLock().lock(); > dropped original implementation along with verification tests. So sole impl You should drop whatever is not necessary in the finishing patches. Line 91: try { Line 92: return allocateNewMacImpl(1)[0]; Line 93: } finally { Line 94: lockObj.writeLock().unlock(); 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()) { > not to obtain lock without a reason. Ok we're not very concurrent environme If so please rewrite as "guard close" Line 207: lockObj.writeLock().lock(); Line 208: try { Line 209: if (!initialized) { Line 210: logInitializationError("Failed to free MAC addresses."); Line 258: } Line 259: } Line 260: Line 261: long size = i - first; Line 262: if (size > Integer.MAX_VALUE) { > former solution used Collections. You should document this.. Line 263: throw new IllegalArgumentException("Not supported that wide ranges(" + size + ")."); Line 264: } Line 265: Line 266: if (numberOfNonMultiCasts > 0) { -- 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
