Nir Soffer has posted comments on this change. Change subject: sampling: do not bail out on errors ......................................................................
Patch Set 3: Code-Review-1 (1 comment) http://gerrit.ovirt.org/#/c/29401/3/vdsm/virt/sampling.py File vdsm/virt/sampling.py: Line 521: except vm.TimeoutError: Line 522: self._log.error("Timeout while sampling stats", Line 523: exc_info=True) Line 524: except Exception as exc: Line 525: self._log.warning("Error while sampling stats: %s", exc) What are the expected errors here? This is not a framework for running user code, when you cannot fail if the user code is broken. This is a thread running specific code, and this code should have expected failure conditions that can be handled in this loop, but we should not handle *any* error, which will just spam the logs with repeating errrors. The expected errors should be handled in place that can raise them. The previous code assume that the only expected error is TimeoutError. Do you think that this assumption is wrong? The old code was logging a traceback and exiting when unexpected error occurred anywhere in this method. The new code will fail silently if an error occur outside the try block inside the while loop. This can be easily fixed by decorating this with @utils.traceback. Line 526: self._stopEvent.wait(self.SAMPLE_INTERVAL_SEC) Line 527: Line 528: @utils.memoized Line 529: def _boot_time(self): -- To view, visit http://gerrit.ovirt.org/29401 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icd05aecc0f66da2bc75f477afc5e17fada0e5f5b Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
