Martin Mucha has posted comments on this change. Change subject: core: extract current implementation to strategy. ......................................................................
Patch Set 5: (6 comments) answers. 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 The reason is very simple. I had to do alternate implementation, which performs better. You have to prove it. And verify if newer implementation works in same way. So I need two coexisting implementations which can be tested via JUnit. Having singleton instance of MacPoolManager does not allow much. That's also the reason why everything has moved. I need all former functionality to remain intact, and also have both this and new implementation accessible in testable way. This way need not to be considered final. But I had to be committed to allow owner of RFE to verify validity and degree of improvement. Otherwise he would not be able to reproduce former and new state and tell whether the improvement is sufficient. 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? fixed. also maxMacsInPool. 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 o why not? -> enables you to specify sole @SuppressWarnings --- will move it & duplicate @SuppressWarnings. Line 64: this.allowDuplicates = allowDuplicates; Line 65: } Line 66: Line 67: @Override Line 95: } Line 96: } Line 97: Line 98: //to allow testing Line 99: List<VmNic> getVmNicInterfacesFromDB() { > I'd prefer passing DAO through the constructor. That would allow mocking th I'd prefer DI. this is not mine code, I'm just refactoring preexisting one — I was supposed to do something specific, not heal all existing wounds. I used this method to enable mocking (now I'm aware of less wrong way how to do it), which is used in following commits. There was formerly bigger patchset which I have to break down to smaller one on developer request. Line 100: return DbFacade.getInstance().getVmNicDao().getAll(); Line 101: } Line 102: Line 103: private String instanceId() { 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? there's semantic difference between 'nested else' and 'guard' (return/throw in 'then' clause ) for code reader. (Kent Beck, implementation patterns.) 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? yes. to allow verifying this method call via mockito. 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
