Tim Armstrong has posted comments on this change. Change subject: IMPALA-5511: Add process start time to debug web page ......................................................................
Patch Set 1: (2 comments) The overall approach seems good, but I had some concerns about the ReadProcStartTime() implementation. http://gerrit.cloudera.org:8080/#/c/7363/1/be/src/util/process-state-info.cc File be/src/util/process-state-info.cc: Line 175: const string& command = Substitute( We're adding a runtime dependence on some additional command-line utilities, which seems brittle. I'm not really sure which versions of these utilities support which arguments, etc. We really want to avoid forking processes after start, since it can cause big spikes in virtual memory usage, e.g. see IMPALA-2294 It seems like we can get the same info directly from stat(): http://pubs.opengroup.org/onlinepubs/009695399/functions/stat.html then format it with strftime() or similar. PS1, Line 176: /proc/$0/ Is the ctime of this directory documented to be the process start time? It looks like it matches on my system but if there's some official documentation it would be good to reference. -- To view, visit http://gerrit.cloudera.org:8080/7363 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I05ae2f80835b1b0e4bc3b38731778ba0871338a4 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Laszlo Gaal <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
