only through vinod's review, benh's coming up next On Thu, Aug 23, 2012 at 12:17 PM, Ben Mahler <[email protected]>wrote:
> > > > 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 > > > > > >
