> On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/common/utils.hpp, line 54 > > <https://reviews.apache.org/r/4944/diff/4/?file=109103#file109103line54> > > > > Now that we're using ostringstream we should include 'sstream'.
done > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/common/utils.hpp, line 229 > > <https://reviews.apache.org/r/4944/diff/4/?file=109103#file109103line229> > > > > Why not call this mktemp? Also, spaces around '=' please. done > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/common/utils.hpp, line 231 > > <https://reviews.apache.org/r/4944/diff/4/?file=109103#file109103line231> > > > > Wait, 'rootDir' is not a dir, it's a path, let's call it 'path'. done > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/common/utils.hpp, line 236 > > <https://reviews.apache.org/r/4944/diff/4/?file=109103#file109103line236> > > > > Why not include strerror here? done > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/common/utils.hpp, line 415 > > <https://reviews.apache.org/r/4944/diff/4/?file=109103#file109103line415> > > > > s/mktempdir/mkdtemp > > s/t="/t = " done > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/common/utils.hpp, line 417 > > <https://reviews.apache.org/r/4944/diff/4/?file=109103#file109103line417> > > > > Now it's a directory, but you might as well still call it path. ;) done > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/common/utils.hpp, line 422 > > <https://reviews.apache.org/r/4944/diff/4/?file=109103#file109103line422> > > > > And strerror would again be helpful. done > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/common/utils.hpp, line 510 > > <https://reviews.apache.org/r/4944/diff/4/?file=109103#file109103line510> > > > > s/reported/reported. done > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/common/utils.hpp, line 700 > > <https://reviews.apache.org/r/4944/diff/4/?file=109103#file109103line700> > > > > const std::string& done > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/common/utils.hpp, line 705 > > <https://reviews.apache.org/r/4944/diff/4/?file=109103#file109103line705> > > > > Formatting is wrong here. fixed > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/common/utils.hpp, line 716 > > <https://reviews.apache.org/r/4944/diff/4/?file=109103#file109103line716> > > > > Formatting. fixed > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/slave/path.hpp, line 38 > > <https://reviews.apache.org/r/4944/diff/4/?file=109105#file109105line38> > > > > 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) s/_LAYOUT/_PATH/ s/getPath/format/ > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/slave/path.hpp, line 56 > > <https://reviews.apache.org/r/4944/diff/4/?file=109105#file109105line56> > > > > Please reformat all opening '{' in this file. done. > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/slave/path.hpp, line 57 > > <https://reviews.apache.org/r/4944/diff/4/?file=109105#file109105line57> > > > > Why not call this enum 'Type'? Seems more descriptive than 'Data'. done > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/slave/path.hpp, line 68 > > <https://reviews.apache.org/r/4944/diff/4/?file=109105#file109105line68> > > > > Kill space. done > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/slave/path.hpp, line 63 > > <https://reviews.apache.org/r/4944/diff/4/?file=109105#file109105line63> > > > > Wait, aren't there only two files? Do we really need a hashmap? hashmap makes it easy when we lookup a specific type of file. > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/slave/path.hpp, line 77 > > <https://reviews.apache.org/r/4944/diff/4/?file=109105#file109105line77> > > > > Again, aren't there only three files? Are these supposed to be the path > > to these files? yes. updated the doc to reflect this. > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/slave/path.hpp, line 85 > > <https://reviews.apache.org/r/4944/diff/4/?file=109105#file109105line85> > > > > 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* s/maxrun/latest > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/slave/path.hpp, line 100 > > <https://reviews.apache.org/r/4944/diff/4/?file=109105#file109105line100> > > > > Space after ',' please. done > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/slave/path.hpp, line 107 > > <https://reviews.apache.org/r/4944/diff/4/?file=109105#file109105line107> > > > > 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. calling it format > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/slave/path.hpp, line 144 > > <https://reviews.apache.org/r/4944/diff/4/?file=109105#file109105line144> > > > > Are these duplicated in the .cpp? Either way, we shouldn't keep them > > here. killed > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/slave/path.cpp, line 22 > > <https://reviews.apache.org/r/4944/diff/4/?file=109106#file109106line22> > > > > Why not: > > > > utils::os::glob(getPath(FRAMEWORK_LAYOUT, root, slaveId, '*')); > > > > You can put that all on a newline. done > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/slave/path.cpp, line 23 > > <https://reviews.apache.org/r/4944/diff/4/?file=109106#file109106line23> > > > > Formatting. fixed > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/slave/path.cpp, line 40 > > <https://reviews.apache.org/r/4944/diff/4/?file=109106#file109106line40> > > > > Again, it would be better to use EXECUTOR_LAYOUT here like above so as > > to keep the semantics encoded in the constants. fixed > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/slave/path.cpp, line 41 > > <https://reviews.apache.org/r/4944/diff/4/?file=109106#file109106line41> > > > > Formatting! done > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/slave/path.cpp, line 51 > > <https://reviews.apache.org/r/4944/diff/4/?file=109106#file109106line51> > > > > For ma tt ing. fixed > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/slave/path.cpp, line 58 > > <https://reviews.apache.org/r/4944/diff/4/?file=109106#file109106line58> > > > > Maybe just skip instead of fail? done > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/slave/path.cpp, line 97 > > <https://reviews.apache.org/r/4944/diff/4/?file=109106#file109106line97> > > > > "No tasks found for run " << run << " of executor " << executorId << " > > of framework " << frameworkId done > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/slave/path.cpp, line 108 > > <https://reviews.apache.org/r/4944/diff/4/?file=109106#file109106line108> > > > > 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. agree, killed > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/slave/path.cpp, line 71 > > <https://reviews.apache.org/r/4944/diff/4/?file=109106#file109106line71> > > > > Seeing this done here like this makes me wonder why it can't be done > > later, when it's needed. (For the others too.) good point, killed > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/slave/path.cpp, line 206 > > <https://reviews.apache.org/r/4944/diff/4/?file=109106#file109106line206> > > > > 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; thanx for the code :) replaced w/ minor modifications. > On 2012-05-21 23:34:36, Benjamin Hindman wrote: > > src/tests/path_tests.cpp, line 44 > > <https://reviews.apache.org/r/4944/diff/4/?file=109109#file109109line44> > > > > If the rootDir is not changing across tests, why not set it up for the > > entire fixture rather than for each test? it is changing...it is created by mktemp - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4944/#review8021 ----------------------------------------------------------- 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 > >
