Roy Golan has posted comments on this change. Change subject: core: VM Monitoring abstract fetching/analyzing/monitoring ......................................................................
Patch Set 17: (10 comments) http://gerrit.ovirt.org/#/c/28662/17/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) { > As define in javadoc for putIfAbsent - "Returns: the previous value associa correct and I don't want to return null. I want to assign manager always. so the ifAbsent isn't enough. and I just realize this code isn't safe at all cause there will be a context switch at the line 465 I'll change this method to something safe 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/17/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: VDS getVds() { > private? public? delete? Done still need to change the clone thing. will supply a separate patch. I hate 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 307: private void logFailureMessage(RuntimeException ex) { Line 308: log.warn( Line 309: "Failed to refresh VDS , cachedVds = '{}' : '{}', error = '{}', continuing.", Line 310: cachedVds.getId(), Line 311: cachedVds.getName()); > change order back to name and then id Done Line 312: log.error("Exception", ex); Line 313: } Line 314: Line 315: private static void logException(final RuntimeException ex) { Line 347: logable.addCustomValue("ErrorMessage", ex.getMessage()); Line 348: logable.updateCallStackFromThrowable(ex); Line 349: AuditLogDirector.log(logable, AuditLogType.VDS_INITIALIZING); Line 350: log.warn( Line 351: "Failed to refresh VDS , cachedVds = {0} : {1}, error = {2}, continuing.", > string is wrong, revert Done Line 352: cachedVds.getId(), Line 353: cachedVds.getName(), Line 354: ex.getMessage()); Line 355: log.debug("Exception", ex); Line 349: AuditLogDirector.log(logable, AuditLogType.VDS_INITIALIZING); Line 350: log.warn( Line 351: "Failed to refresh VDS , cachedVds = {0} : {1}, error = {2}, continuing.", Line 352: cachedVds.getId(), Line 353: cachedVds.getName(), > change order back Done Line 354: ex.getMessage()); Line 355: log.debug("Exception", ex); Line 356: final int VDS_RECOVERY_TIMEOUT_IN_MINUTES = Config.<Integer> getValue(ConfigValues.VdsRecoveryTimeoutInMinutes); Line 357: String jobId = SchedulerUtilQuartzImpl.getInstance().scheduleAOneTimeJob(this, "onTimerHandleVdsRecovering", new Class[0], Line 765: case Down: Line 766: break; Line 767: case NonResponsive: Line 768: log.debug( Line 769: "Failed to refresh VDS , cachedVds = '{}' : '{}', VDS Network Error, continuing. '{}'", > why changing the msg? i think should be reverted Done Line 770: cachedVds.getId(), Line 771: cachedVds.getName(), Line 772: e.getMessage()); Line 773: break; Line 772: e.getMessage()); Line 773: break; Line 774: default: Line 775: log.warn( Line 776: "Failed to refresh VDS , cachedVds = '{}' : '{}', VDS Network Error, continuing. '{}'", > same here Done Line 777: cachedVds.getId(), Line 778: cachedVds.getName(), Line 779: e.getMessage()); Line 780: } http://gerrit.ovirt.org/#/c/28662/17/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VmsListFetcher.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VmsListFetcher.java: Line 59: if (getList.getSucceeded()) { Line 60: vdsmVms = (Map<Guid, VmInternalData>) getList.getReturnValue(); Line 61: onFetchVms(); Line 62: } Line 63: onError(); > shouldnt this be inside 'else'? yes changed already Line 64: } Line 65: Line 66: protected void onFetchVms() { Line 67: dbVms = getVmDao().getAllRunningByVds(vdsManager.getVdsId()); http://gerrit.ovirt.org/#/c/28662/17/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VmsStatisticsFetcher.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VmsStatisticsFetcher.java: Line 25: if (getStats.getSucceeded()) { Line 26: vdsmVms = (Map<Guid, VmInternalData>) getStats.getReturnValue(); Line 27: onFetchVms(); Line 28: } Line 29: onError(); > same yes Line 30: } Line 31: Line 32: @Override Line 33: protected void gatherChangedVms(VM dbVm, VmInternalData vdsmVm) { http://gerrit.ovirt.org/#/c/28662/17/backend/manager/modules/vdsbroker/src/test/java/org/ovirt/engine/core/vdsbroker/Data.java File backend/manager/modules/vdsbroker/src/test/java/org/ovirt/engine/core/vdsbroker/Data.java: Line 15: Line 16: import java.util.ArrayList; Line 17: import java.util.HashMap; Line 18: Line 19: public enum Data { > maybe find a better name Done Line 20: EXTERNAL_VM("0") { Line 21: @Override Line 22: Pair<VM, VmInternalData> build() { Line 23: return pairOf(null, createVmInternalData()); -- 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: 17 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
