----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4944/#review8021 -----------------------------------------------------------
src/common/utils.hpp <https://reviews.apache.org/r/4944/#comment17419> Now that we're using ostringstream we should include 'sstream'. src/common/utils.hpp <https://reviews.apache.org/r/4944/#comment17420> Why not call this mktemp? Also, spaces around '=' please. src/common/utils.hpp <https://reviews.apache.org/r/4944/#comment17421> Wait, 'rootDir' is not a dir, it's a path, let's call it 'path'. src/common/utils.hpp <https://reviews.apache.org/r/4944/#comment17422> Why not include strerror here? src/common/utils.hpp <https://reviews.apache.org/r/4944/#comment17423> s/mktempdir/mkdtemp s/t="/t = " src/common/utils.hpp <https://reviews.apache.org/r/4944/#comment17424> Now it's a directory, but you might as well still call it path. ;) src/common/utils.hpp <https://reviews.apache.org/r/4944/#comment17425> And strerror would again be helpful. src/common/utils.hpp <https://reviews.apache.org/r/4944/#comment17428> s/reported/reported. src/common/utils.hpp <https://reviews.apache.org/r/4944/#comment17426> const std::string& src/common/utils.hpp <https://reviews.apache.org/r/4944/#comment17427> Formatting is wrong here. src/common/utils.hpp <https://reviews.apache.org/r/4944/#comment17429> Good use of Result! src/common/utils.hpp <https://reviews.apache.org/r/4944/#comment17430> Formatting. src/slave/path.hpp <https://reviews.apache.org/r/4944/#comment17437> I think the suffix _PATH reads better to me. Likewise, I think either just 'get' or 'format' reads better than 'getPath' as well. For example, the following two lines read pretty naturally to me: get(FRAMEWORK_PATH, root, slaveId, frameworkId) format(FRAMEWORK_PATH, root, slaveId, frameworkId) src/slave/path.hpp <https://reviews.apache.org/r/4944/#comment17440> Please reformat all opening '{' in this file. src/slave/path.hpp <https://reviews.apache.org/r/4944/#comment17439> Why not call this enum 'Type'? Seems more descriptive than 'Data'. src/slave/path.hpp <https://reviews.apache.org/r/4944/#comment17441> Wait, aren't there only two files? Do we really need a hashmap? src/slave/path.hpp <https://reviews.apache.org/r/4944/#comment17431> Kill space. src/slave/path.hpp <https://reviews.apache.org/r/4944/#comment17442> Again, aren't there only three files? Are these supposed to be the path to these files? src/slave/path.hpp <https://reviews.apache.org/r/4944/#comment17443> I think 'latest' is more descriptive than maxrun. I also think the cleanest way to do this would really be, when someone wants the latest run: RunState state; int latest = sort(state.runs.keys()).back(); But perhaps that's still not possible given our current libraries. *sigh* src/slave/path.hpp <https://reviews.apache.org/r/4944/#comment17444> Space after ',' please. src/slave/path.hpp <https://reviews.apache.org/r/4944/#comment17438> I think you should call this 'get' or even just 'format', since that's what it will ultimately get called once we move the format stuff into strings. src/slave/path.hpp <https://reviews.apache.org/r/4944/#comment17432> Are these duplicated in the .cpp? Either way, we shouldn't keep them here. src/slave/path.cpp <https://reviews.apache.org/r/4944/#comment17435> Why not: utils::os::glob(getPath(FRAMEWORK_LAYOUT, root, slaveId, '*')); You can put that all on a newline. src/slave/path.cpp <https://reviews.apache.org/r/4944/#comment17433> Formatting. src/slave/path.cpp <https://reviews.apache.org/r/4944/#comment17434> This is great: the way this is being done "procedurally" now enables great log lines like this! src/slave/path.cpp <https://reviews.apache.org/r/4944/#comment17446> Again, it would be better to use EXECUTOR_LAYOUT here like above so as to keep the semantics encoded in the constants. src/slave/path.cpp <https://reviews.apache.org/r/4944/#comment17445> Formatting! src/slave/path.cpp <https://reviews.apache.org/r/4944/#comment17447> For ma tt ing. src/slave/path.cpp <https://reviews.apache.org/r/4944/#comment17436> Maybe just skip instead of fail? src/slave/path.cpp <https://reviews.apache.org/r/4944/#comment17448> Seeing this done here like this makes me wonder why it can't be done later, when it's needed. (For the others too.) src/slave/path.cpp <https://reviews.apache.org/r/4944/#comment17449> "No tasks found for run " << run << " of executor " << executorId << " of framework " << frameworkId src/slave/path.cpp <https://reviews.apache.org/r/4944/#comment17450> Again, it seems like recording the TaskID is the most important part, and getting the paths to these files can be done later, wherever it is needed. src/slave/path.cpp <https://reviews.apache.org/r/4944/#comment17451> What about something like this here instead: int run = 0; Result<list<string> > paths = glob(format(EXECUTOR_PATH, directory, id, frameworkId, executorId, '*')); CHECK(!paths.isError()) << paths.error(); if (paths.isSome()) { foreach (const string& path, paths.get()) { Try<int> temp = numify<int>(basename(path)); if (temp.isError()) { continue; } run = max(run, temp.get()); } } string result = format(EXECUTOR_PATH, directory, id, frameworkId, executorId, run + 1); bool created = mkdir(result); CHECK(created) << ...; return result; src/slave/path.cpp <https://reviews.apache.org/r/4944/#comment17453> These are fine for now but if they are only being used in one place my preference will be that this code just get's inlined there. src/slave/slave.cpp <https://reviews.apache.org/r/4944/#comment17454> Glad to see all of this go! It's been wreaking havoc on my ability to sleep well at night. ;) src/tests/path_tests.cpp <https://reviews.apache.org/r/4944/#comment17455> If the rootDir is not changing across tests, why not set it up for the entire fixture rather than for each test? - Benjamin On 2012-05-18 17:05:42, Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4944/ > ----------------------------------------------------------- > > (Updated 2012-05-18 17:05:42) > > > Review request for mesos, Benjamin Hindman and John Sirois. > > > Summary > ------- > > See summary. > > This diff is based on the previous utils patch > (https://reviews.apache.org/r/4899). I still need to update that patch with > john's comments. > > Added tests. > > > Diffs > ----- > > src/tests/path_tests.cpp PRE-CREATION > src/slave/slave.hpp 08a29d8 > src/slave/slave.cpp e08be3b > src/slave/path.cpp PRE-CREATION > src/master/http.cpp f72ceb7 > src/slave/path.hpp PRE-CREATION > src/Makefile.am 333234d > src/common/utils.hpp 1d81e21 > > Diff: https://reviews.apache.org/r/4944/diff > > > Testing > ------- > > make check > > > Thanks, > > Vinod > >
