Roy Golan has posted comments on this change. Change subject: core: VM Monitoring abstract fetching/analyzing/monitoring ......................................................................
Patch Set 14: (4 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 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(); > Please initialize in constructor Martin is it a general format thing? I'm asking generally cause you commented that in couple other places. 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; 2. its potentially duplicated code when I'll add a second constructor. Line 93: Line 94: private VdsManager(VDS vds) { Line 95: log.info("Entered VdsManager constructor"); Line 96: cachedVds = vds; Line 160: new Object[0], Line 161: refreshRate, Line 162: refreshRate, Line 163: TimeUnit.MILLISECONDS); Line 164: // register push provider and consumer > TODO ? Done Line 165: } Line 166: Line 167: private void initVdsBroker() { Line 168: log.info("Initialize vdsBroker '{}:{}'", cachedVds.getHostName(), cachedVds.getPort()); Line 289: } Line 290: } Line 291: Line 292: VDS getVds() { Line 293: return cachedVds.clone(); > Do we need to clone? that's a good point. I meant the clone to be deep so the cached VDS couldn't be tempered. since VDS isn't implementing Clonable there isn't any use for that. I'll reexamine this. Line 294: } Line 295: public String getVdsName() { Line 296: return cachedVds.getName(); Line 297: } -- 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
