Mike Kolesnik has posted comments on this change. Change subject: core: extract current implementation to strategy. ......................................................................
Patch Set 5: (5 comments) Please notice you also had comments which you didn't take care of on patch set 4. http://gerrit.ovirt.org/#/c/26400/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacPoolManager.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacPoolManager.java: Line 4: Line 5: import org.ovirt.engine.core.common.config.Config; Line 6: import org.ovirt.engine.core.common.config.ConfigValues; Line 7: Line 8: public class MacPoolManager { If there's only an empty shell left, I don't see why you need to extract to strategy at all. The MacPoolManager is already an API by itself, you could just replace it's internals. Also if you're already extracting to strategy, I don't see a reason to also move the thread-synchronization logic (as it's also duplicated later on to the other strategy). Line 9: Line 10: private final MacPoolManagerStrategy macPoolManagerStrategy; Line 11: Line 12: private static final MacPoolManager INSTANCE = new MacPoolManager(); http://gerrit.ovirt.org/#/c/26400/5/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 29: private static final Log log = LogFactory.getLog(MacPoolManagerOriginal.class); Line 30: Line 31: private final Integer maxMacsInPool; Line 32: private final String ranges; Line 33: private final Boolean allowDuplicates; Would it make sense to have null here? If not, this should be primitive. Line 34: Line 35: /** Line 36: * A Map that holds the allocated MAC addresses as keys, and counters as values. These MAC addresses were taken from Line 37: * the range defined by the user in {@link org.ovirt.engine.core.common.config.ConfigValues#MacPoolRanges} Line 59: this.maxMacsInPool = maxMacsInPool; Line 60: this.ranges = ranges; Line 61: availableMacs = new HashSet<>(); Line 62: allocatedCustomMacs = new CaseInsensitiveMap(); Line 63: allocatedMacs = new CaseInsensitiveMap(); Why initialize these fields here? They're not dependant on the parameters of the constructor. Line 64: this.allowDuplicates = allowDuplicates; Line 65: } Line 66: Line 67: @Override Line 133: Line 134: if (macAddresses.size() + availableMacs.size() > maxMacsInPool) { Line 135: availableMacs.addAll(macAddresses.subList(0, maxMacsInPool - availableMacs.size())); Line 136: throw new MacPoolExceededMaxException(); //TODO MM: what's this? why returning boolean value then? Line 137: } else { Why nest in else block? Line 138: availableMacs.addAll(macAddresses); Line 139: // System.out.println("!!"+availableMacs.size()); Line 140: return true; Line 141: } Line 175: } Line 176: return true; Line 177: } Line 178: Line 179: void logMacPoolEmpty() { Did you intentionally leave this in package aceess modifier? Line 180: AuditLogableBase logable = new AuditLogableBase(); Line 181: AuditLogDirector.log(logable, AuditLogType.MAC_POOL_EMPTY); Line 182: } Line 183: -- To view, visit http://gerrit.ovirt.org/26400 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I75498bb68ec6dbe28650027116bf44a0797faf93 Gerrit-PatchSet: 5 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
