> On Feb. 5, 2013, 10:27 p.m., Ben Mahler wrote:
> > This is SO MUCH CLEANER!
> >
> > Two notes:
> > (1) Seems like Error could also take a second argument:
> > Error(string m, Try<X> try) : message(m + ": " + try.error())
> >
> > But that gets a bit implicit:
> > Try<string> read = os::read("/foo");
> > return Error("Failed to read '/foo'", read);
> > vs.
> > return Error("Failed to read '/foo': " + read.error());
> >
> > (2) Nearly all of my comments here are for additional cleanup in the
> > existing code. It would be awesome if you got all these, but feel free to
> > drop if it's too much.
Regardless of the implicitness here, what would be the semantics if 'read'
actually isn't an error? (Or is that what you meant by 'implicit'?)
> On Feb. 5, 2013, 10:27 p.m., Ben Mahler wrote:
> > src/common/values.cpp, line 56
> > <https://reviews.apache.org/r/9306/diff/1/?file=255493#file255493line56>
> >
> > Maybe in this review we should clean up these messages? They seem
> > redundant and inconsistent as they are now.
> >
> > I think it's fair to expect the caller to print this out as:
> >
> > LOG(ERROR) << "Failed to parse '" << text << "': " << parse.error();
> >
> > In which case the current messages won't read so nicely.
Done and done.
> On Feb. 5, 2013, 10:27 p.m., Ben Mahler wrote:
> > src/files/files.cpp, line 417
> > <https://reviews.apache.org/r/9306/diff/1/?file=255495#file255495line417>
> >
> > I should have made this:
> >
> > s/result/realpath
> > "Failed to get realpath: " + realpath.error()
I did "Failed to determine canonical path ...".
> On Feb. 5, 2013, 10:27 p.m., Ben Mahler wrote:
> > src/linux/cgroups.cpp, line 104
> > <https://reviews.apache.org/r/9306/diff/1/?file=255497#file255497line104>
> >
> > too bad ifstream doesn't provide something akin to strerror
Added a TODO to switch to os::read for better error information.
> On Feb. 5, 2013, 10:27 p.m., Ben Mahler wrote:
> > src/linux/cgroups.cpp, line 323
> > <https://reviews.apache.org/r/9306/diff/1/?file=255497#file255497line323>
> >
> > FWICT, It turns out it's not part of the spec!
> >
> > If we really want the strerror message, then we can't use ifstream. I
> > think we do want strerror, given how many places here we just print an
> > error message without knowing why it fails.
> >
> > Maybe a TODO since this happens in many places in the cgroups code.
As it turns out there is already a comment here explaining that we can't use
os::read because it does lseek. As we've talked about before, we need an
implementation of os::read(path) which does the "expected" semantics of just
reading data until EOF. I've added a TODO here nonetheless.
> On Feb. 5, 2013, 10:27 p.m., Ben Mahler wrote:
> > src/linux/cgroups.cpp, line 677
> > <https://reviews.apache.org/r/9306/diff/1/?file=255497#file255497line677>
> >
> > this Option<string> reads backwards from all other error checking, TODO
> > to fix?
Ha, we talked about this before ... check out the old thread. The TL;DR; is
that this is supposed to really act like a "macro" that many of these functions
needs to perform. The verify routine returns _some_ fully constructed error
string if there is an error that can be directly returned via
Error(error.get()). I didn't like the idea of returning a Try<Nothing> and then
pulling out the error and returning it via Error(verification.error()) because
we try and always prepend something to the error string (e.g., Error("Failed to
verify: " + verification.error())) and in these cases we wouldn't/shouldn't
need to. Given your reaction here, it sounds like this should get changed, but
I'm not going to take that on in this review.
> On Feb. 5, 2013, 10:27 p.m., Ben Mahler wrote:
> > src/linux/cgroups.cpp, line 769
> > <https://reviews.apache.org/r/9306/diff/1/?file=255497#file255497line769>
> >
> > s/a/the/ ?
Changed to "Failed to stop traversing file system" (in above and below errors
too).
> On Feb. 5, 2013, 10:27 p.m., Ben Mahler wrote:
> > src/slave/cgroups_isolation_module.cpp, line 281
> > <https://reviews.apache.org/r/9306/diff/1/?file=255506#file255506line281>
> >
> > single quote instead and single quote the hierarchy?
> On Feb. 5, 2013, 10:27 p.m., Ben Mahler wrote:
> > third_party/libprocess/include/stout/net.hpp, line 30
> > <https://reviews.apache.org/r/9306/diff/1/?file=255519#file255519line30>
> >
> > Can we add "because libcurl is not available"?
I changed the entire comment to "libcurl is not available", so a concatenated
message will probably read: "Failed to download 'resource': libcurl is not
available".
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9306/#review16134
-----------------------------------------------------------
On Feb. 8, 2013, 5:49 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9306/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2013, 5:49 a.m.)
>
>
> Review request for mesos, Vinod Kone and Ben Mahler.
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/common/values.cpp 8aebc0b57de110e14c55c5b6d543f17165cfd397
> src/detector/detector.cpp ca921151a1fc1a57d265b3439d07a8c45e920f21
> src/files/files.cpp 60b567eb62e84ccc99b0b1978733a0ea96560813
> src/flags/parse.hpp df7e3480c1f5422d185c86cc5f34e7b87b024a2f
> src/linux/cgroups.cpp 03b31e7309b9dd65f00d3b0da2abb81ddaaeea43
> src/linux/fs.cpp ea74c3b935dc716fd53827c8a7eb7010961e7aa6
> src/linux/proc.cpp 8a0fc48dc9769df35d682ece477246b2df2fc0d4
> src/log/coordinator.cpp d9f67bd8b07e6f8b3e022c21f1764d298e314594
> src/log/log.hpp 151eebd5d665993dfaac399553a9a315c093762b
> src/log/replica.cpp 392cb15991657d8e79637e1c4bc21ec813e3117d
> src/logging/logging.cpp e9d778f840f2d6713dbb99d0a8844033be9f0e5f
> src/master/frameworks_manager.cpp 189a7e3b4811643baf285ad29f939453257c705f
> src/sched/sched.cpp f1eeab6f12ee300d77013c6a4ba62ccd7fdb0d1d
> src/slave/cgroups_isolation_module.cpp
> 63cefc33cf34eebb82db5d8448b751be8652fa36
> src/state/leveldb.cpp 6137b5d089dc9ea4b60fd56153b72e14691df3d8
> src/state/serializer.hpp 1915cefb102526fbad330b090b8c788bea65ed84
> src/state/zookeeper.cpp 4c684797791bb2158d31573d2d47b0da70283bc6
> src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed
> src/tests/stout_tests.cpp fffef3e42774e9b090a098a219a92e642d303d27
> src/zookeeper/group.cpp e74538e3dfa6951178f3e4b49dfd94374a0aa28c
> src/zookeeper/url.hpp c62539b30503a94ed82e11fe84d988c122d36bb6
> third_party/libprocess/include/process/http.hpp
> 40248dab3df59b322ad9c5a4858961ef471779fd
> third_party/libprocess/include/stout/duration.hpp
> caf7ce42b34dfb74206d1f10aafabaaec47e7dfc
> third_party/libprocess/include/stout/error.hpp PRE-CREATION
> third_party/libprocess/include/stout/format.hpp
> fc349903d31d6e31a8147e3a6be87b37698f28f0
> third_party/libprocess/include/stout/fs.hpp
> 9e62a1b91bc9fac092818ffb3c8bcec46b0bd26d
> third_party/libprocess/include/stout/gzip.hpp
> 6ff1288683a0d3b05098c772945cc2a8b73f8b01
> third_party/libprocess/include/stout/net.hpp
> 1909046017da005a5e7c64d525bb3c741d5ad436
> third_party/libprocess/include/stout/numify.hpp
> 2a59c9269902541d82ba86a2d159984f5bbc6b73
> third_party/libprocess/include/stout/os.hpp
> 712fb209061384b8c045875ef8898a6efd778514
> third_party/libprocess/include/stout/protobuf.hpp
> 25d4634bbc51d14ea78c1a922353f0ce52e458df
>
> Diff: https://reviews.apache.org/r/9306/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
