> On 2012-05-08 23:16:57, Benjamin Hindman wrote: > > src/common/utils.hpp, line 190 > > <https://reviews.apache.org/r/4899/diff/5/?file=106624#file106624line190> > > > > Related to the previous comment, I don't like something called create > > that also truncates. > > > > If the use case is to have a new truncated file, one can simply do: > > > > utils::os::rm(path); > > utils::os::touch(path); > > > > That is, I'd prefer this was the 'touch' abstraction we know and love, > > without the truncate. I would consider the ability to pass a 'bool > > truncate' flag to 'touch', but I'm not sure it's necessary.
ok..removed O_TRUNC option and renamed create to touch. > On 2012-05-08 23:16:57, Benjamin Hindman wrote: > > src/common/utils.hpp, line 196 > > <https://reviews.apache.org/r/4899/diff/5/?file=106624#file106624line196> > > > > We really shouldn't be logging here (and everywhere else in utils, > > since they all return Try or Result). killed. here and everywhere else > On 2012-05-08 23:16:57, Benjamin Hindman wrote: > > src/common/utils.hpp, line 217 > > <https://reviews.apache.org/r/4899/diff/5/?file=106624#file106624line217> > > > > Shouldn't need this line, just use 'strerror(errno)' on the next line. done > On 2012-05-08 23:16:57, Benjamin Hindman wrote: > > src/common/utils.hpp, line 253 > > <https://reviews.apache.org/r/4899/diff/5/?file=106624#file106624line253> > > > > Okay, I reneg my previous comment. I think it makes sense to use Result > > for read because there are three possibilities: (1) the data; (2) an error; > > (3) EOF. We can represent EOF as Result::none. fixed for all versions of read(). do we want Try or Result for write(). Technically, write shouldnt return a none() but I like the symmetry. > On 2012-05-08 23:16:57, Benjamin Hindman wrote: > > src/common/utils.hpp, line 265 > > <https://reviews.apache.org/r/4899/diff/5/?file=106624#file106624line265> > > > > Did you want an 'else if' here? And you should kill this check anyway > > and return a Result::none below where you check if length == 0. killed > On 2012-05-08 23:16:57, Benjamin Hindman wrote: > > src/common/utils.hpp, line 303 > > <https://reviews.apache.org/r/4899/diff/5/?file=106624#file106624line303> > > > > Kill. killed > On 2012-05-08 23:16:57, Benjamin Hindman wrote: > > src/common/utils.hpp, line 304 > > <https://reviews.apache.org/r/4899/diff/5/?file=106624#file106624line304> > > > > s/open file./open " + path done. here and everywhere else. > On 2012-05-08 23:16:57, Benjamin Hindman wrote: > > src/common/utils.hpp, line 543 > > <https://reviews.apache.org/r/4899/diff/5/?file=106624#file106624line543> > > > > Kill. killed > On 2012-05-08 23:16:57, Benjamin Hindman wrote: > > src/common/utils.hpp, line 555 > > <https://reviews.apache.org/r/4899/diff/5/?file=106624#file106624line555> > > > > What if it's an error? If your intention is that it will never be an > > error, make this a CHECK first before you do a .get(). added the check. > On 2012-05-08 23:16:57, Benjamin Hindman wrote: > > src/common/utils.hpp, line 748 > > <https://reviews.apache.org/r/4899/diff/5/?file=106624#file106624line748> > > > > Right, here, using Result makes sense (like the read above). Just all > > the other places didn't make sense. fixed > On 2012-05-08 23:16:57, Benjamin Hindman wrote: > > src/slave/slave.cpp, line 737 > > <https://reviews.apache.org/r/4899/diff/5/?file=106625#file106625line737> > > > > s/rea/Try< :) > On 2012-05-08 23:16:57, Benjamin Hindman wrote: > > src/tests/utils_tests.cpp, line 89 > > <https://reviews.apache.org/r/4899/diff/5/?file=106627#file106627line89> > > > > UtilsTest should really be a fixture which makes sure the file gets > > deleted even if the test fails (see https://reviews.apache.org/r/4981 for > > an example). done - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4899/#review7702 ----------------------------------------------------------- On 2012-05-04 00:28:13, Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4899/ > ----------------------------------------------------------- > > (Updated 2012-05-04 00:28:13) > > > Review request for mesos and John Sirois. > > > Summary > ------- > > We can now read/write strings to a file. > > Some common utils for working with protobuf objects. > > > Diffs > ----- > > src/Makefile.am cd503a8 > src/common/protobuf_utils.hpp PRE-CREATION > src/common/utils.hpp 1d81e21 > src/slave/slave.cpp 09a8396 > src/tests/protobuf_io_tests.cpp 22f37ac > src/tests/utils_tests.cpp 49ec107 > > Diff: https://reviews.apache.org/r/4899/diff > > > Testing > ------- > > make check > > > Thanks, > > Vinod > >
