Omer Frenkel has posted comments on this change. Change subject: core: VM Monitoring abstract fetching/analyzing/monitoring ......................................................................
Patch Set 19: (5 comments) http://gerrit.ovirt.org/#/c/28662/19/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 456: Line 457: return null; Line 458: } Line 459: Line 460: public VmManager getVmManager(Guid vmId) { are you going to change this impl? Line 461: if (vmManagers.containsKey(vmId)) { Line 462: return vmManagers.get(vmId); Line 463: } else { Line 464: VmManager value = new VmManager(vmId); http://gerrit.ovirt.org/#/c/28662/19/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 287: ).perform(); Line 288: } Line 289: } Line 290: Line 291: public VDS getVds() { can you please explain why this is needed? i thought you are going to delete it Line 292: //TODO use deep clone for defensive copying. Make VDS implement Clonable Line 293: return cachedVds.clone(); Line 294: } Line 295: public String getVdsName() { Line 305: } Line 306: Line 307: private void logFailureMessage(RuntimeException ex) { Line 308: log.warn( Line 309: "Failed to refresh VDS , cachedVds = '{}' : '{}', error = '{}', continuing.", please change message to use "vds" and not "cachedVds" Line 310: cachedVds.getName(), Line 311: cachedVds.getId()); Line 312: log.error("Exception", ex); Line 313: } Line 307: private void logFailureMessage(RuntimeException ex) { Line 308: log.warn( Line 309: "Failed to refresh VDS , cachedVds = '{}' : '{}', error = '{}', continuing.", Line 310: cachedVds.getName(), Line 311: cachedVds.getId()); you are missing the ex.getMsg Line 312: log.error("Exception", ex); Line 313: } Line 314: Line 315: private static void logException(final RuntimeException ex) { Line 909: Line 910: public static void cancelRecoveryJob(Guid vdsId) { Line 911: String jobId = recoveringJobIdMap.remove(vdsId); Line 912: if (jobId != null) { Line 913: log.info("Cancelling the recovery from crash timer for VDS '{}' because cachedVds started initializing", vdsId); please change message to use "vds" and not "cachedVds" Line 914: try { Line 915: SchedulerUtilQuartzImpl.getInstance().deleteJob(jobId); Line 916: } catch (Exception e) { Line 917: log.warn("Failed deleting job '{}' at cancelRecoveryJob: {}", jobId, e.getMessage()); -- 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: 19 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
