-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9306/#review16134
-----------------------------------------------------------
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.
src/common/values.cpp
<https://reviews.apache.org/r/9306/#comment34488>
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.
src/files/files.cpp
<https://reviews.apache.org/r/9306/#comment34490>
I should have made this:
s/result/realpath
"Failed to get realpath: " + realpath.error()
src/linux/cgroups.cpp
<https://reviews.apache.org/r/9306/#comment34491>
too bad ifstream doesn't provide something akin to strerror
src/linux/cgroups.cpp
<https://reviews.apache.org/r/9306/#comment34492>
s/directory at "/directory '"/
s/": "/"': "/
src/linux/cgroups.cpp
<https://reviews.apache.org/r/9306/#comment34494>
s/at "/'"/
s/": "/"': "/
src/linux/cgroups.cpp
<https://reviews.apache.org/r/9306/#comment34495>
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.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/9306/#comment34497>
single quotes around hierarchy
src/linux/cgroups.cpp
<https://reviews.apache.org/r/9306/#comment34496>
single quotes around dir
src/linux/cgroups.cpp
<https://reviews.apache.org/r/9306/#comment34498>
single quotes around hierarchy
src/linux/cgroups.cpp
<https://reviews.apache.org/r/9306/#comment34499>
single quotes around hierarchy
src/linux/cgroups.cpp
<https://reviews.apache.org/r/9306/#comment34500>
ditto
src/linux/cgroups.cpp
<https://reviews.apache.org/r/9306/#comment34501>
ditto
src/linux/cgroups.cpp
<https://reviews.apache.org/r/9306/#comment34503>
this Option<string> reads backwards from all other error checking, TODO to
fix?
src/linux/cgroups.cpp
<https://reviews.apache.org/r/9306/#comment34504>
s/a/the/ ?
src/linux/cgroups.cpp
<https://reviews.apache.org/r/9306/#comment34505>
Should we include value.get() here at least?
src/linux/fs.cpp
<https://reviews.apache.org/r/9306/#comment34506>
single quote path
src/linux/fs.cpp
<https://reviews.apache.org/r/9306/#comment34507>
single quote these?
src/linux/fs.cpp
<https://reviews.apache.org/r/9306/#comment34509>
single quote target?
src/linux/proc.cpp
<https://reviews.apache.org/r/9306/#comment34512>
single quote?
src/linux/proc.cpp
<https://reviews.apache.org/r/9306/#comment34514>
single quote?
src/master/frameworks_manager.cpp
<https://reviews.apache.org/r/9306/#comment34515>
"Framework does not exist."?
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9306/#comment34516>
Thanks for catching these CHECK_SOMEs!
single quote?
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9306/#comment34517>
single quote?
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9306/#comment34518>
single quote instead and single quote the hierarchy?
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9306/#comment34519>
single quote instead?
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9306/#comment34520>
single quote instead?
src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/9306/#comment34521>
single quote instead?
src/state/zookeeper.cpp
<https://reviews.apache.org/r/9306/#comment34523>
Is compression easy now with gzip available? Perhaps update this TODO to
reflect that?
src/zookeeper/group.cpp
<https://reviews.apache.org/r/9306/#comment34524>
s/base/basename/ ?
third_party/libprocess/include/stout/gzip.hpp
<https://reviews.apache.org/r/9306/#comment34525>
"Failed to initialize zlib: " + ...
third_party/libprocess/include/stout/gzip.hpp
<https://reviews.apache.org/r/9306/#comment34526>
"Failed to clean up zlib: " + ...
third_party/libprocess/include/stout/gzip.hpp
<https://reviews.apache.org/r/9306/#comment34527>
"Failed to initialize zlib: " + ...
third_party/libprocess/include/stout/gzip.hpp
<https://reviews.apache.org/r/9306/#comment34528>
"Failed to clean up zlib: " + ...
third_party/libprocess/include/stout/net.hpp
<https://reviews.apache.org/r/9306/#comment34529>
Can we add "because libcurl is not available"?
third_party/libprocess/include/stout/numify.hpp
<https://reviews.apache.org/r/9306/#comment34530>
thanks!
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/9306/#comment34531>
single quotes
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/9306/#comment34534>
single quotes?
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/9306/#comment34536>
This is more of 'directory' not being a directory. It may still exist (as a
file, link, etc).
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/9306/#comment34537>
Add "on non-linux systems" ?
third_party/libprocess/include/stout/protobuf.hpp
<https://reviews.apache.org/r/9306/#comment34540>
single quotes
- Ben Mahler
On Feb. 5, 2013, 5:57 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9306/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2013, 5:57 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/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
>
>
