----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49243/#review141289 -----------------------------------------------------------
Looks pretty good and close to being shippable. I also included a sample diff while playing around with the code that has the fix to these issues, feel free to apply it (might contain some style errors though): https://gist.github.com/hatred/a6b95c9195f8d331e9948639309e3ca6 src/files/files.cpp (line 113) <https://reviews.apache.org/r/49243/#comment206756> Nit: Kill this comment. Having it once in the header is enough. src/files/files.cpp (line 545) <https://reviews.apache.org/r/49243/#comment206757> Missing Indent. src/files/files.cpp (line 566) <https://reviews.apache.org/r/49243/#comment206758> We removed this in the `LIST_FILES` review too since the invariant check was not improving readability much. See discussing on #http-api for more details. src/files/files.cpp (line 598) <https://reviews.apache.org/r/49243/#comment206797> Not yours. We don't end error messages with a new line character. Can you verify if the WebUI/Pailer depends on these new line characters? If not, would be good to get rid of them in a separate patch. src/files/files.cpp (line 642) <https://reviews.apache.org/r/49243/#comment206790> hmm, seems like you needed to make this change because the `defer` lambda cannot be mutable? I am fine with moving all this logic to a continuation function `_read()` that would be invoked by `read()`. We can rename the route handler function to `__read()`. src/files/files.cpp (lines 647 - 650) <https://reviews.apache.org/r/49243/#comment206802> Move this after L642 src/files/files.cpp (lines 677 - 678) <https://reviews.apache.org/r/49243/#comment206799> Why did you change this to `defer`? src/files/files.cpp (line 678) <https://reviews.apache.org/r/49243/#comment206800> Space between () and -> - Anand Mazumdar On July 8, 2016, 11:49 a.m., zhou xing wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49243/ > ----------------------------------------------------------- > > (Updated July 8, 2016, 11:49 a.m.) > > > Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone. > > > Bugs: mesos-5515 > https://issues.apache.org/jira/browse/mesos-5515 > > > Repository: mesos > > > Description > ------- > > This method helps to readFiles based on offset, length > and path, which can be used for implementing master/agent's > READ_FILE operator v1 API. > > > Diffs > ----- > > src/files/files.hpp c36ecfb729d70aa0fb194fc8e285a8fb2e297ef2 > src/files/files.cpp c8991c4ffcddc2c196d96cdfec104cd7a87aac34 > > Diff: https://reviews.apache.org/r/49243/diff/ > > > Testing > ------- > > make > make check > > > Thanks, > > zhou xing > >
