Nir Soffer has posted comments on this change. Change subject: host stats: Collect stats from online cpu cores only ......................................................................
Patch Set 9: Code-Review-1 (6 comments) https://gerrit.ovirt.org/#/c/46269/9/tests/hoststatsTests.py File tests/hoststatsTests.py: Line 107: Line 108: Line 109: class HostStatsThreadTests(TestCaseBase): Line 110: Line 111: def _expectedResult(self, node_index=0): Having default value only make the tests harder to understand. Better require all caller to specify the node_index, will make the tests more readable. Also _expectedResult is meaningless, isn't this cpuStats dict? Line 112: return { Line 113: str(node_index): { Line 114: 'cpuIdle': '100.00', Line 115: 'cpuSys': '0.00', Line 125: }, Line 126: 1: { Line 127: 'cpus': [1] Line 128: } Line 129: } Why is this so verbose? What is wrong with: {0: {'cpus': [0]}, 1: {'cpus': [1]}} And why do we need a function to return this in tests? Line 130: Line 131: def testCpuCoreStats(self): Line 132: cpu_sample = {'user': 1.0, 'sys': 2.0} Line 133: Line 137: with MonkeyPatchScope([(caps, 'getNumaTopology', Line 138: self._fakeNumaTopology)]): Line 139: result = hoststats._get_cpu_core_stats(first_sample, last_sample) Line 140: self.assertEqual(result['0'], self._expectedResult()['0']) Line 141: self.assertEqual(result['1'], self._expectedResult(1)['1']) You are using only one dict in each assert, so I don't understand why _expectedResults is needed at all. This is much more clear: expected = {'cpuIdle': 100.0, ...} self.assertEqual(result['1'], expected) Line 142: Line 143: def testSkipStatsOnMissingFirstSample(self): Line 144: cpu_sample = {'user': 1.0, 'sys': 2.0} Line 145: Line 149: with MonkeyPatchScope([(caps, 'getNumaTopology', Line 150: self._fakeNumaTopology)]): Line 151: self.assertEqual( Line 152: hoststats._get_cpu_core_stats(first_sample, last_sample), Line 153: self._expectedResult()) Again we are using one dict, better inline it. Line 154: Line 155: def testSkipStatsOnMissingLastSample(self): Line 156: cpu_sample = {'user': 1.0, 'sys': 2.0} Line 157: Line 161: with MonkeyPatchScope([(caps, 'getNumaTopology', Line 162: self._fakeNumaTopology)]): Line 163: self.assertEqual( Line 164: hoststats._get_cpu_core_stats(first_sample, last_sample), Line 165: self._expectedResult()) Same Line 166: Line 167: def testOutputWithNoSamples(self): Line 168: expected = { Line 169: 'cpuIdle': 100.0, 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 re Please never check None with TypeError - write code that reveal your intent. 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
