Yevgeny Zaspitsky has posted comments on this change. Change subject: core: MacPoolManager strategy, alternative implementation, test for benchmarks ......................................................................
Patch Set 6: (2 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{ > no problem, I can change it. But this was actually used in original impleme http://www.ovirt.org/Building_Ovirt_Engine/IDE You can find there how to setup your IDE (Eclipse/IntelliJ). IMHO that's the correct way. Mike, please approve. Line 22: Line 23: private static final Log log = LogFactory.getLog(MacPoolManagerRanges.class); Line 24: Line 25: private final int maxMacsInPool; Line 74: } Line 75: } Line 76: Line 77: //to allow testing; may be fixed after DI is used. Line 78: List<VmNic> getVmNicInterfacesFromDB() { > not me. a) only if a case of large number of dependencies -> the class does too much, split it into few smaller ones b) your argument just proves (a) -> group methods into smaller classes c) having a class initialized through its constructor allows to keep the class immutable, which is thread safe and much easier to to keep its logic simple IMHO. d) having a single constructor with all dependencies makes it easy to find what the class dependencies are in a single place. e) I'm not sure what do you mean by "Use of DI is proper way". To me injecting dependencies through the constructor is the better way as it leaves the option of using the class without a DI framework (e.g. in a unit test). Line 79: return DbFacade.getInstance().getVmNicDao().getAll(); Line 80: } Line 81: Line 82: //to allow testing; may be fixed after DI is used. -- 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
