Francesco Romani has posted comments on this change. Change subject: host stats: Collect stats from online cpu cores only ......................................................................
Patch Set 9: Code-Review-1 (1 comment) unfortunately has to be -1. I'm actually fine with all of this patch except one implementation detail. I'm fully aware that EAFP vs LBYL is mostly matter of taste (and that the first is actually more pythonic), nevertheless the stats subsystem is heavily LBYL, and I'd like to keep it that way, OR to change all of it at once. For the near/mid term we chosse the LBYL way, hence the score and the requirement. https://gerrit.ovirt.org/#/c/46269/9/vdsm/virt/hoststats.py File vdsm/virt/hoststats.py: Line 103: last_sample.cpuCores.getCoreSample(cpu_core)[mode] - Line 104: first_sample.cpuCores.getCoreSample(cpu_core)[mode] Line 105: ) % JIFFIES_BOUND Line 106: return ("%.2f" % (jiffies / interval)) Line 107: except TypeError: this is the only thing that bothers me in this patch. We spent some time removing except TypError and except AttributeError, and I'd like to avoid to reintroduce them so early, and without a very good reason to do so. I believe that here we can make the code a little less dense and increase a bit in clarity, adding a few ifs to guard for None. Line 108: return None Line 109: Line 110: cpu_core_stats = {} Line 111: for node_index, numa_node in six.iteritems(caps.getNumaTopology()): -- To view, visit https://gerrit.ovirt.org/46269 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia9c247f9138e02a9230a0849a04cb2e1705e7fac Gerrit-PatchSet: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Roman Mohr <rm...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Omer Frenkel <d.mosqu...@gmail.com> Gerrit-Reviewer: Roman Mohr <rm...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
