----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63368/#review196340 -----------------------------------------------------------
3rdparty/libprocess/Makefile.am Lines 213 (patched) <https://reviews.apache.org/r/63368/#comment275910> Please tabs 3rdparty/libprocess/Makefile.am Lines 303 (patched) <https://reviews.apache.org/r/63368/#comment275911> Please tabs 3rdparty/libprocess/include/process/memory_profiler.hpp Lines 36 (patched) <https://reviews.apache.org/r/63368/#comment275912> No need to use the FQN here. 3rdparty/libprocess/include/process/memory_profiler.hpp Lines 51-52 (patched) <https://reviews.apache.org/r/63368/#comment275914> "How long"... what? Can you please be more verbose here? Also you mean request parameters, can you say this explicitly? 3rdparty/libprocess/include/process/memory_profiler.hpp Lines 57-58 (patched) <https://reviews.apache.org/r/63368/#comment275926> Blank comment line in-between, please 3rdparty/libprocess/include/process/memory_profiler.hpp Lines 64-66 (patched) <https://reviews.apache.org/r/63368/#comment275928> Please write a leading comment explaining what these functions return, i.e., last _finished_ state. 3rdparty/libprocess/include/process/memory_profiler.hpp Lines 67 (patched) <https://reviews.apache.org/r/63368/#comment275927> No need for process prefix, here and below. 3rdparty/libprocess/include/process/memory_profiler.hpp Lines 81 (patched) <https://reviews.apache.org/r/63368/#comment275933> What kind of statistics is this? Is this related to the last / active state? Or is it accumulated across all states since process launch? Can you please elaborate? 3rdparty/libprocess/include/process/memory_profiler.hpp Lines 104 (patched) <https://reviews.apache.org/r/63368/#comment275980> Let's call this guy `stopAndGenerateRawProfile()` for clarity. 3rdparty/libprocess/include/process/memory_profiler.hpp Lines 124 (patched) <https://reviews.apache.org/r/63368/#comment275925> ~~One shot one kill~~one line one declaration, please. Any reason to keep these fellas as `Try`s? Please also expand comments on these fields. For example, ``` // A timestamp of a last successful raw dump. Option<time_t> rawId; ``` 3rdparty/libprocess/include/process/memory_profiler.hpp Lines 130 (patched) <https://reviews.apache.org/r/63368/#comment275918> Let's make it `const`. You will likely need a static or free function that read the env var. 3rdparty/libprocess/src/memory_profiler.cpp Lines 52 (patched) <https://reviews.apache.org/r/63368/#comment275959> Please backtick type, variable, class, and function names in comments. 3rdparty/libprocess/src/memory_profiler.cpp Lines 198-240 (patched) <https://reviews.apache.org/r/63368/#comment275960> Can we combine these two with in a `Result<T> updateJemallocSetting(const char* name, const T& value)`? 3rdparty/libprocess/src/memory_profiler.cpp Lines 245 (patched) <https://reviews.apache.org/r/63368/#comment275963> Here you convert `string` to `Path` in order to convert it back at a call site. I suggest to make `getRawProfilePath()` return `string. Also, there is no need in leading slashes. And since you already have a section for literals, maybe put file names there? 3rdparty/libprocess/src/memory_profiler.cpp Lines 261-271 (patched) <https://reviews.apache.org/r/63368/#comment275966> It's not obvious that updating a setting triggers a dump. Can you please leave a note here? 3rdparty/libprocess/src/memory_profiler.cpp Lines 275 (patched) <https://reviews.apache.org/r/63368/#comment275967> One shot one... 3rdparty/libprocess/src/memory_profiler.cpp Lines 313 (patched) <https://reviews.apache.org/r/63368/#comment275968> Why newline? 3rdparty/libprocess/src/memory_profiler.cpp Lines 326-330 (patched) <https://reviews.apache.org/r/63368/#comment275971> Formatting. 3rdparty/libprocess/src/memory_profiler.cpp Lines 334 (patched) <https://reviews.apache.org/r/63368/#comment275970> Let's make it a lambda in `start()` 3rdparty/libprocess/src/memory_profiler.cpp Lines 347 (patched) <https://reviews.apache.org/r/63368/#comment275969> Let's make it a lambda in `stop()`. 3rdparty/libprocess/src/memory_profiler.cpp Lines 355-357 (patched) <https://reviews.apache.org/r/63368/#comment275972> For clarity, let's instead make it ``` Option<time_t> extractIdFromRequest(const process::http::Request& request) ``` Then at the call site you can say: `extractIdFromRequest(r).getOrElse(fallback.getOrElse(0))` 3rdparty/libprocess/src/memory_profiler.cpp Lines 361 (patched) <https://reviews.apache.org/r/63368/#comment275974> `strtol`? 3rdparty/libprocess/src/memory_profiler.cpp Lines 372 (patched) <https://reviews.apache.org/r/63368/#comment275975> Consider using an alias for `process::http` in this file. 3rdparty/libprocess/src/memory_profiler.cpp Lines 382 (patched) <https://reviews.apache.org/r/63368/#comment275976> kill "end" 3rdparty/libprocess/src/memory_profiler.cpp Lines 387 (patched) <https://reviews.apache.org/r/63368/#comment275958> This looks like a general pattern we use in libprocess. Can we add an overload for `route()` and fix other places? Somehting like this: ``` template <typename T> void route( const std::string& name, const Option<std::string>& realm, const Option<std::string>& help, Future<http::Response> (T::*method)( const http::Request&, const Option<http::authentication::Principal>&), const RouteOptions& options = RouteOptions()) { if (realm.isSome()) { route(name, realm.get(), help, handler, options); } else { // Note that we use dynamic_cast here so a process can use // multiple inheritance if it sees so fit (e.g., to implement // multiple callback interfaces). HttpRequestHandler handler = lambda::bind(method, dynamic_cast<T*>(this), lambda::_1, None()); route(name, help, handler, options); } } ``` 3rdparty/libprocess/src/memory_profiler.cpp Lines 395-398 (patched) <https://reviews.apache.org/r/63368/#comment275957> I understand that you try to save some characters, but I'd argue and explicit `HELP` declaration is superior. Moreover, it is consistent to the rest of the codebase. Please define `static const std::string *_HELP();` functions 3rdparty/libprocess/src/memory_profiler.cpp Lines 686-690 (patched) <https://reviews.apache.org/r/63368/#comment275981> This looks to me that a situation when `rawId` is reset but the old file exists is possible. It is probably fine for now, but deserves a comment. 3rdparty/libprocess/src/memory_profiler.cpp Lines 714 (patched) <https://reviews.apache.org/r/63368/#comment275982> Don't you need to check `graphId.isSome()`? 3rdparty/libprocess/src/memory_profiler.cpp Lines 807 (patched) <https://reviews.apache.org/r/63368/#comment275983> Do you check that the file actually exists before streaming it to the user? I don't see it neither here, nor in `generateGraph()`. You probably want to put such check into `generateGraph()` and reset `graphId` in case file is gone or can't be accessed. - Alexander Rukletsov On Oct. 27, 2017, 7:04 p.m., Benno Evers wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63368/ > ----------------------------------------------------------- > > (Updated Oct. 27, 2017, 7:04 p.m.) > > > Review request for mesos, Alexander Rukletsov and Benjamin Mahler. > > > Repository: mesos > > > Description > ------- > > This class exposes profiling functionality of jemalloc memory allocator > when it is detected to be the memory allocator of the current process. > > In particular, it gives developers an easy method to collect and > access heap profiles which report which pieces of code were > responsible for allocating memory. > > > Diffs > ----- > > 3rdparty/libprocess/Makefile.am ba9e78148932304ce1961b88e5f97180abd35586 > 3rdparty/libprocess/include/process/memory_profiler.hpp PRE-CREATION > 3rdparty/libprocess/include/process/process.hpp > 8b8323f5fb4617aa9c22bbdb56af3d1cb0b06ea3 > 3rdparty/libprocess/src/memory_profiler.cpp PRE-CREATION > 3rdparty/libprocess/src/process.cpp > 2126c272a69bfa53b4b3cbbb1a55a3f81a5da8ad > > > Diff: https://reviews.apache.org/r/63368/diff/2/ > > > Testing > ------- > > > Thanks, > > Benno Evers > >
