Martin Peřina has posted comments on this change. Change subject: core: VM Monitoring abstract fetching/analyzing/monitoring ......................................................................
Patch Set 14: (2 comments) http://gerrit.ovirt.org/#/c/28662/14/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/ResourceManager.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/ResourceManager.java: Line 457: } Line 458: Line 459: public VmManager getVmManager(Guid vmId) { Line 460: if (vmManagers.contains(vmId)) { Line 461: return vmManagers.get(vmId); > cause its going to create an instance without a need True, but it's atomic. Wouldn't we get into problem using this non atomic operation without synchronization? Line 462: } else { Line 463: VmManager value = new VmManager(vmId); Line 464: vmManagers.put(vmId, value); Line 465: return value; http://gerrit.ovirt.org/#/c/28662/14/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java: Line 88: private IVdsServer vdsProxy; Line 89: private boolean mBeforeFirstRefresh = true; Line 90: private HostMonitoring hostMonitoring; Line 91: private boolean monitoringNeeded; Line 92: private List<Pair<VM, VmInternalData>> lastVmsList = Collections.emptyList(); > Martin is it a general format thing? I'm asking generally cause you comment > Martin is it a general format thing? I'm asking generally cause you commented > that in > couple other places. Well, yes, IMHO it's more readable to have initialization in constructors than spread over the class. With lots of attributes constructor initialization readability becomes more visible. > since this class and the VMs/HostMonitoring classes(currently) have long list > of > members, initializing instances at the cstr have 2 disadvateges: > > 1. its making the class longer; True > 2. its potentially duplicated code when I'll add a second constructor. Can be easily used by constructor chaining, which IMHO is a preferred way with multiple contsructors Line 93: Line 94: private VdsManager(VDS vds) { Line 95: log.info("Entered VdsManager constructor"); Line 96: cachedVds = vds; -- To view, visit http://gerrit.ovirt.org/28662 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1adf0a95007140e89b080b5160ba93e340ee3ba6 Gerrit-PatchSet: 14 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
