Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12657 )
Change subject: KUDU-2721: support ranges in CPU lists ...................................................................... Patch Set 6: (4 comments) http://gerrit.cloudera.org:8080/#/c/12657/6/src/kudu/gutil/sysinfo.cc File src/kudu/gutil/sysinfo.cc: http://gerrit.cloudera.org:8080/#/c/12657/6/src/kudu/gutil/sysinfo.cc@168 PS6, Line 168: CHECK(SlurpSmallTextFile("/sys/devices/system/cpu/possible", buf, arraysize(buf))); > interestingly, on the machines I have access to, the number in 'possible' i Maybe I should decouple this part of the change, since we haven't seen any issues in practice with using /present. Impala creates a buffer pool arena per CPU, but won't use it. So there's some overhead in terms of metrics and any code that iterates over all arenas, but probably not a big deal. http://gerrit.cloudera.org:8080/#/c/12657/6/src/kudu/gutil/sysinfo.cc@174 PS6, Line 174: int ParseMaxCpuIndex(const char* str) { > should probably be static We expose it in the header for testing, so it has to have external linkage. http://gerrit.cloudera.org:8080/#/c/12657/6/src/kudu/gutil/sysinfo.cc@183 PS6, Line 183: // newline or comma if the input is valid. > Instead of the hand-rolled parsing code, maybe it's easier to just move the Doesn't that have other potential pitfalls if MaxCPUIndex() gets called too early during the initialization phase? There is a comment warning about this up the top of the file. http://gerrit.cloudera.org:8080/#/c/12657/6/src/kudu/gutil/sysinfo.cc@201 PS6, Line 201: long start_idx = strtol(range_start, nullptr, 10); > warning: consider replacing 'long' with 'int64' [google-runtime-int] I'm going to ignore these because it's for the result of strtol() -- To view, visit http://gerrit.cloudera.org:8080/12657 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I97311bfbcf70bea069e941b6e7f4f015fb781b3f Gerrit-Change-Number: 12657 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 05 Mar 2019 21:19:13 +0000 Gerrit-HasComments: Yes
