Arik Hadas has posted comments on this change.
Change subject: core: create a data structure for all Runtime updates logics
......................................................................
Patch Set 1: (2 inline comments)
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VmRepository.java
Line 12: import
org.ovirt.engine.core.common.businessentities.network.VmNetworkInterface;
Line 13: import org.ovirt.engine.core.compat.Guid;
Line 14: import
org.ovirt.engine.core.vdsbroker.vdsbroker.entities.VmInternalData;
Line 15:
Line 16: public class VmRepository {
I think that it's a good idea to make the class visibility narrower, IMO in
this case it can be inner class in VmRepositoryUpdater
even if this class is not designed to have logic, returning the internal
collections is not a good idea in terms of encapsulation - why is
VmRepositoryUpdater needs to know that there's a collection of VMs-to-rerun
inside VmRepository? maybe all the VMs-to-rerun and auto-Vms-to-run are stored
in the same collection with different flags to save memory? what he only needs
to have is for example a method that let him add VM to rerun in the repository.
in order to return the objects that were already inserted we can return
unmodifiable collection as allon suggested or cloned collection (note that if
you return unmodifiable collection and remove the setter-methods as we talked
about, you have to add "insert methods" right?)
Line 17: private Map<Guid, VM> vmDict;
Line 18: private Map<Guid, VmInternalData> runningVms;
Line 19:
Line 20: private List<Guid> vmsToRerun;
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VmRepositoryUpdater.java
Line 29:
Line 30:
Line 31: public class VmRepositoryUpdater {
Line 32:
Line 33: private VmRepository repo;
repository is better than repo, but I meant more coding convention - I'm used
to see that instance of not-general class is called like its class when the
first letter is lower-case, for example instance of MyFoo -> myFoo (like the
vdsManager field below). besides being consistent, it should be more readable
that way in general when you encounter such fields inside methods
Line 34: private VdsManager vdsManager;
Line 35:
Line 36: private static final Log log =
LogFactory.getLog(VmRepositoryUpdater.class);
Line 37:
--
To view, visit http://gerrit.ovirt.org/12273
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I434b1429dfa184f7a8212843ad7ee2c5a196c783
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Roy Golan <rgo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Arik Hadas <aha...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Roy Golan <rgo...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
