> On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > src/files/files.hpp, line 59 > > <https://reviews.apache.org/r/6617/diff/3/?file=143735#file143735line59> > > > > i'm guessing u made this public to make it visible for testing? if yes, > > please add a comment saying so.
done > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > src/files/files.cpp, line 66 > > <https://reviews.apache.org/r/6617/diff/3/?file=143736#file143736line66> > > > > s/std::string/string done > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > src/files/files.cpp, lines 77-78 > > <https://reviews.apache.org/r/6617/diff/3/?file=143736#file143736line77> > > > > Kill this done > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > src/files/files.cpp, line 105 > > <https://reviews.apache.org/r/6617/diff/3/?file=143736#file143736line105> > > > > removing trailing "/" is a good utility function to have in strings seems succinct enough as it though using strings::remove > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > src/files/files.cpp, line 134 > > <https://reviews.apache.org/r/6617/diff/3/?file=143736#file143736line134> > > > > when does this happen? never :) > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > src/files/files.cpp, line 144 > > <https://reviews.apache.org/r/6617/diff/3/?file=143736#file143736line144> > > > > push this down to where its used done > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > src/files/files.cpp, line 313 > > <https://reviews.apache.org/r/6617/diff/3/?file=143736#file143736line313> > > > > I dont' completely understand what's happening here. Can you add more > > comments/docs please? added an explanation with an example, hopefully it makes sense now? > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > src/master/slaves_manager.cpp, line 835 > > <https://reviews.apache.org/r/6617/diff/3/?file=143739#file143739line835> > > > > can't u use numify for this? yep, alright, more refactoring then > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > src/master/slaves_manager.cpp, line 921 > > <https://reviews.apache.org/r/6617/diff/3/?file=143739#file143739line921> > > > > ditto yep, done > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > src/tests/files_tests.cpp, lines 19-21 > > <https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line19> > > > > reorder? done > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > src/tests/files_tests.cpp, line 29 > > <https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line29> > > > > new line done > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > src/tests/files_tests.cpp, line 56 > > <https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line56> > > > > for this test, why do u need something to the file? I don't really, removed > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > src/tests/files_tests.cpp, lines 107-108 > > <https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line107> > > > > are u supposed to test something here? nope, since it returns void, I'm just checking it doesn't crash I think this test will help against regressions though as well > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > src/tests/files_tests.cpp, line 132 > > <https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line132> > > > > s/4/strlen("body")/ done > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > src/tests/files_tests.cpp, line 151 > > <https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line151> > > > > why these blocks? so that I don't need to name things like request1, request2, ... I can just a nested namespaces instead > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > src/tests/files_tests.cpp, line 145 > > <https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line145> > > > > not needed? needed, mkdir is non-recursive > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > src/tests/files_tests.cpp, line 261 > > <https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line261> > > > > not necessary not recursive, so needed > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > src/tests/files_tests.cpp, line 179 > > <https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line179> > > > > s/5/strlen("three")/ done > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > src/tests/files_tests.cpp, lines 177-180 > > <https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line177> > > > > you should probably factor this out into a helper function. since I only do this twice, let's not bother > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > src/tests/files_tests.cpp, lines 213-216 > > <https://reviews.apache.org/r/6617/diff/3/?file=143742#file143742line213> > > > > it would be factor this out to a helper too. done! this really trimmed down the test code :) > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > src/tests/utils.hpp, line 92 > > <https://reviews.apache.org/r/6617/diff/3/?file=143743#file143743line92> > > > > 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. ah I see, killed my version > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > src/tests/utils.hpp, line 101 > > <https://reviews.apache.org/r/6617/diff/3/?file=143743#file143743line101> > > > > wait..dont we already have this abstraction in stout/os? > > if so, can you use that instead? ah yes, done > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > third_party/libprocess/include/stout/os.hpp, line 326 > > <https://reviews.apache.org/r/6617/diff/3/?file=143749#file143749line326> > > > > kill line done > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > third_party/libprocess/include/stout/os.hpp, lines 333-339 > > <https://reviews.apache.org/r/6617/diff/3/?file=143749#file143749line333> > > > > this seems like !isdir(), why do we need a different function? not quite.. !isDir does not imply isFile, and !isFile does not imply isDir, since the dir/file can be non existent > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > third_party/libprocess/include/stout/os.hpp, line 338 > > <https://reviews.apache.org/r/6617/diff/3/?file=143749#file143749line338> > > > > kill line done > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > third_party/libprocess/src/decoder.hpp, line 82 > > <https://reviews.apache.org/r/6617/diff/3/?file=143752#file143752line82> > > > > why static? ignoring, given benh's comments > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > third_party/libprocess/src/decoder.hpp, line 303 > > <https://reviews.apache.org/r/6617/diff/3/?file=143752#file143752line303> > > > > need an assert(!decoder->failure) ? nope, just returning 0 now instead of pulling out the decoder > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > third_party/libprocess/src/decoder.hpp, lines 345-363 > > <https://reviews.apache.org/r/6617/diff/3/?file=143752#file143752line345> > > > > add asserts for failure? well failure gets triggered in decode(), then on message begin the assert will check for failure, failure cannot be true in these functions > On Aug. 22, 2012, 9:54 p.m., Vinod Kone wrote: > > third_party/libprocess/src/decoder.hpp, line 372 > > <https://reviews.apache.org/r/6617/diff/3/?file=143752#file143752line372> > > > > new line done - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6617/#review10643 ----------------------------------------------------------- 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 > >
