----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6617/#review10643 -----------------------------------------------------------
src/files/files.hpp <https://reviews.apache.org/r/6617/#comment22758> make this virtual src/files/files.hpp <https://reviews.apache.org/r/6617/#comment22799> i'm guessing u made this public to make it visible for testing? if yes, please add a comment saying so. src/files/files.cpp <https://reviews.apache.org/r/6617/#comment22759> s/std::string/string src/files/files.cpp <https://reviews.apache.org/r/6617/#comment22761> Kill this src/files/files.cpp <https://reviews.apache.org/r/6617/#comment22768> removing trailing "/" is a good utility function to have in strings src/files/files.cpp <https://reviews.apache.org/r/6617/#comment22771> when does this happen? src/files/files.cpp <https://reviews.apache.org/r/6617/#comment22774> push this down to where its used src/files/files.cpp <https://reviews.apache.org/r/6617/#comment22780> I dont' completely understand what's happening here. Can you add more comments/docs please? src/master/slaves_manager.cpp <https://reviews.apache.org/r/6617/#comment22783> can't u use numify for this? src/master/slaves_manager.cpp <https://reviews.apache.org/r/6617/#comment22784> ditto src/tests/configurator_tests.cpp <https://reviews.apache.org/r/6617/#comment22793> i like this abstraction! src/tests/files_tests.cpp <https://reviews.apache.org/r/6617/#comment22787> reorder? src/tests/files_tests.cpp <https://reviews.apache.org/r/6617/#comment22788> new line src/tests/files_tests.cpp <https://reviews.apache.org/r/6617/#comment22795> for this test, why do u need something to the file? src/tests/files_tests.cpp <https://reviews.apache.org/r/6617/#comment22798> are u supposed to test something here? src/tests/files_tests.cpp <https://reviews.apache.org/r/6617/#comment22800> s/4/strlen("body")/ src/tests/files_tests.cpp <https://reviews.apache.org/r/6617/#comment22801> not needed? src/tests/files_tests.cpp <https://reviews.apache.org/r/6617/#comment22803> why these blocks? src/tests/files_tests.cpp <https://reviews.apache.org/r/6617/#comment22805> you should probably factor this out into a helper function. src/tests/files_tests.cpp <https://reviews.apache.org/r/6617/#comment22804> s/5/strlen("three")/ src/tests/files_tests.cpp <https://reviews.apache.org/r/6617/#comment22806> it would be factor this out to a helper too. src/tests/files_tests.cpp <https://reviews.apache.org/r/6617/#comment22807> not necessary src/tests/utils.hpp <https://reviews.apache.org/r/6617/#comment22809> i see why you were creating parent directories here. we already have this function in stout/os, which acts like 'mkdir-p'. use that instead. src/tests/utils.hpp <https://reviews.apache.org/r/6617/#comment22808> wait..dont we already have this abstraction in stout/os? if so, can you use that instead? third_party/libprocess/include/stout/os.hpp <https://reviews.apache.org/r/6617/#comment22812> kill line third_party/libprocess/include/stout/os.hpp <https://reviews.apache.org/r/6617/#comment22815> this seems like !isdir(), why do we need a different function? third_party/libprocess/include/stout/os.hpp <https://reviews.apache.org/r/6617/#comment22813> kill line third_party/libprocess/src/decoder.hpp <https://reviews.apache.org/r/6617/#comment22816> why static? third_party/libprocess/src/decoder.hpp <https://reviews.apache.org/r/6617/#comment22818> need an assert(!decoder->failure) ? third_party/libprocess/src/decoder.hpp <https://reviews.apache.org/r/6617/#comment22820> add asserts for failure? third_party/libprocess/src/decoder.hpp <https://reviews.apache.org/r/6617/#comment22821> new line - Vinod Kone On Aug. 22, 2012, 1:37 a.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6617/ > ----------------------------------------------------------- > > (Updated Aug. 22, 2012, 1:37 a.m.) > > > Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu. > > > Description > ------- > > Implementing the file abstraction and http endpoints for file reading / > browsing. > > > This addresses bug MESOS-255. > https://issues.apache.org/jira/browse/MESOS-255 > > > Diffs > ----- > > src/Makefile.am aaceee3 > src/files/files.hpp d0cab91 > src/files/files.cpp d4080d4 > src/logging/logging.cpp 6909b0b > src/master/http.cpp c480bc6 > src/master/slaves_manager.cpp e25efd0 > src/slave/http.cpp a1f7926 > src/tests/configurator_tests.cpp c2f5aa0 > src/tests/files_tests.cpp PRE-CREATION > src/tests/utils.hpp 83d5daa > src/webui/master/static/controllers.js 1606e64 > third_party/libprocess/include/process/http.hpp 8424ca6 > third_party/libprocess/include/process/io.hpp 6a40b18 > third_party/libprocess/include/stout/hashmap.hpp 51bdea0 > third_party/libprocess/include/stout/json.hpp 25dbcf4 > third_party/libprocess/include/stout/os.hpp b1eceb3 > third_party/libprocess/include/stout/path.hpp 80d9bc6 > third_party/libprocess/include/stout/stringify.hpp ad2f2fa > third_party/libprocess/src/decoder.hpp 105fe5d > third_party/libprocess/src/encoder.hpp ac4886e > third_party/libprocess/src/process.cpp 78069bf > third_party/libprocess/src/statistics.cpp d05b327 > > Diff: https://reviews.apache.org/r/6617/diff/ > > > Testing > ------- > > Added files_tests.cpp > make check > > > Thanks, > > Ben Mahler > >
