Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/8546 )
Change subject: IMPALA-5624: Replace "ls -l" with opendir() in ProcessStateInfo ...................................................................... Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/8546/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8546/4//COMMIT_MSG@11 PS4, Line 11: api nit: acronyms are usually all caps http://gerrit.cloudera.org:8080/#/c/8546/4/be/src/util/process-state-info.h File be/src/util/process-state-info.h: http://gerrit.cloudera.org:8080/#/c/8546/4/be/src/util/process-state-info.h@77 PS4, Line 77: /// File Descriptors information will be read from /proc/pid/fd. nit: /proc/self/fd http://gerrit.cloudera.org:8080/#/c/8546/4/be/src/util/process-state-info.cc File be/src/util/process-state-info.cc: http://gerrit.cloudera.org:8080/#/c/8546/4/be/src/util/process-state-info.cc@155 PS4, Line 155: does nit: do not count. http://gerrit.cloudera.org:8080/#/c/8546/4/be/src/util/process-state-info.cc@156 PS4, Line 156: self nit: either use getpid() here to be consistent with the rest of the file, or change the other occurrences to /proc/self/* http://gerrit.cloudera.org:8080/#/c/8546/4/be/src/util/process-state-info.cc@159 PS4, Line 159: dir Can you try to think of better variable names? You have a DIR called d, and something else called dir. DIR is really a directory stream, and readdir returns directory entries. http://gerrit.cloudera.org:8080/#/c/8546/4/be/src/util/process-state-info.cc@160 PS4, Line 160: while ((dir = readdir(d)) != nullptr) ++fd_count; Please add a brief comment why your usage of readdir is thread-safe here. http://gerrit.cloudera.org:8080/#/c/8546/4/be/src/util/process-state-info.cc@163 PS4, Line 163: std:: You shouldn't need std:: here -- To view, visit http://gerrit.cloudera.org:8080/8546 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibffae8069a62e100abbfa7d558b49040b095ddc0 Gerrit-Change-Number: 8546 Gerrit-PatchSet: 4 Gerrit-Owner: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Comment-Date: Fri, 17 Nov 2017 23:55:31 +0000 Gerrit-HasComments: Yes
