----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63372/#review201770 -----------------------------------------------------------
docs/memory-profiling.md Lines 6 (patched) <https://reviews.apache.org/r/63372/#comment283361> IIRC, the convention is to use `#` only for the title, i.e., this line, meaning you should add one `#` to each section below. Also, the title above should coincide with this line. docs/memory-profiling.md Lines 22 (patched) <https://reviews.apache.org/r/63372/#comment283354> Please links in "[]()" format. docs/memory-profiling.md Lines 27 (patched) <https://reviews.apache.org/r/63372/#comment283366> You mention here memory profiling but actually speak about jemalloc. How about the following. "To make memory profiling possible, jemalloc memory allocator is required. There are two ways..." or "A prerequisite for memory profiling is a suitable allocator. We currently support jemalloc, which can be connected via one of the following ways..." docs/memory-profiling.md Lines 31 (patched) <https://reviews.apache.org/r/63372/#comment283356> the bundled version? docs/memory-profiling.md Lines 31-32 (patched) <https://reviews.apache.org/r/63372/#comment283357> The end of the sentence sounds a bit weird. Can you please rephrase? docs/memory-profiling.md Lines 34 (patched) <https://reviews.apache.org/r/63372/#comment283358> What do you mean under present? On the build host? On the target host? Maybe rephrasing this saying something like "Users can provide their own jemalloc build via..." docs/memory-profiling.md Lines 36-37 (patched) <https://reviews.apache.org/r/63372/#comment283359> Maybe decorate this as a `**NOTE:**` to capture people's attention? docs/memory-profiling.md Lines 41 (patched) <https://reviews.apache.org/r/63372/#comment283360> Please back tick types. docs/memory-profiling.md Lines 50 (patched) <https://reviews.apache.org/r/63372/#comment283362> s/,/:/ docs/memory-profiling.md Lines 53-54 (patched) <https://reviews.apache.org/r/63372/#comment283364> Let's explain a bit more, users will appreciate! Something like this: "Switching to the jemalloc allocator alone is not enough to get memory dumps, the binaries itself should have memory profiling enabled. To do this, start..." docs/memory-profiling.md Lines 71 (patched) <https://reviews.apache.org/r/63372/#comment283380> This section lacks some guidance regarding the UX. Please mention somewhere in this section things like 1. There can only be one active profiling run. 2. Data from a single, most recently finished profiling run are stored. 3. Fumbling with jemalloc memory profiling out of band is unsupported and can lead to weird results. docs/memory-profiling.md Lines 78 (patched) <https://reviews.apache.org/r/63372/#comment283369> What are the other ways? Are they alternatives or you discourage using them? docs/memory-profiling.md Lines 82-84 (patched) <https://reviews.apache.org/r/63372/#comment283382> Please tell people they will get an `id` of the progiling run that can be used to uniqly identify it. docs/memory-profiling.md Lines 98 (patched) <https://reviews.apache.org/r/63372/#comment283370> comma after i.e. docs/memory-profiling.md Lines 100-102 (patched) <https://reviews.apache.org/r/63372/#comment283372> No need to put this link onto a separate line. "Check out [the official jemalloc documentation](link) for the description of the file format" docs/memory-profiling.md Lines 108 (patched) <https://reviews.apache.org/r/63372/#comment283373> Back tick `jeprof` docs/memory-profiling.md Lines 109 (patched) <https://reviews.apache.org/r/63372/#comment283375> Period at the end. docs/memory-profiling.md Lines 111 (patched) <https://reviews.apache.org/r/63372/#comment283374> Dito re `jeprof` docs/memory-profiling.md Lines 122-124 (patched) <https://reviews.apache.org/r/63372/#comment283376> Is `dot` tool required as well? docs/memory-profiling.md Lines 125 (patched) <https://reviews.apache.org/r/63372/#comment283377> Is this blank line necessary? docs/memory-profiling.md Lines 127-128 (patched) <https://reviews.apache.org/r/63372/#comment283378> I don't think this adds much value : ), you can directly start with the next paragraph. docs/memory-profiling.md Lines 151 (patched) <https://reviews.apache.org/r/63372/#comment283383> maybe prepend the line with `$` to indicate it is a copypasteable command? docs/memory-profiling.md Lines 167 (patched) <https://reviews.apache.org/r/63372/#comment283381> this? docs/memory-profiling.md Lines 168 (patched) <https://reviews.apache.org/r/63372/#comment283384> Does \~\~\~{.bash} work? docs/memory-profiling.md Lines 183 (patched) <https://reviews.apache.org/r/63372/#comment283387> Maybe also warn people in this section that they can mess up and cannot blame us? docs/memory-profiling.md Lines 192 (patched) <https://reviews.apache.org/r/63372/#comment283385> Let's have an inline link in form []() for consistency. docs/memory-profiling.md Lines 194 (patched) <https://reviews.apache.org/r/63372/#comment283386> Please be locally consistent and deside between `run-time` and `runtime` : ) - Alexander Rukletsov On April 13, 2018, 6:49 p.m., Benno Evers wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63372/ > ----------------------------------------------------------- > > (Updated April 13, 2018, 6:49 p.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Repository: mesos > > > Description > ------- > > Added documentation for memory profiling. > > > Diffs > ----- > > docs/memory-profiling.md PRE-CREATION > > > Diff: https://reviews.apache.org/r/63372/diff/7/ > > > Testing > ------- > > > Thanks, > > Benno Evers > >
