Return-Path: X-Original-To: apmail-incubator-mesos-dev-archive@minotaur.apache.org Delivered-To: apmail-incubator-mesos-dev-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 6E6C897A2 for ; Mon, 21 May 2012 23:34:38 +0000 (UTC) Received: (qmail 39563 invoked by uid 500); 21 May 2012 23:34:38 -0000 Delivered-To: apmail-incubator-mesos-dev-archive@incubator.apache.org Received: (qmail 39537 invoked by uid 500); 21 May 2012 23:34:38 -0000 Mailing-List: contact mesos-dev-help@incubator.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: mesos-dev@incubator.apache.org Delivered-To: mailing list mesos-dev@incubator.apache.org Received: (qmail 39515 invoked by uid 99); 21 May 2012 23:34:38 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 21 May 2012 23:34:38 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 61CD41C406F; Mon, 21 May 2012 23:34:36 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============0943537104478240972==" MIME-Version: 1.0 Subject: Re: Review Request: Refactored path utilities into a separate file From: "Benjamin Hindman" To: "Benjamin Hindman" , "John Sirois" Date: Mon, 21 May 2012 23:34:36 -0000 Message-ID: <20120521233436.3611.49759@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org X-ReviewRequest-URL: https://reviews.apache.org/r/4944/ Cc: "Vinod Kone" , "mesos" In-Reply-To: <20120518170542.811.40057@reviews.apache.org> References: <20120518170542.811.40057@reviews.apache.org> --===============0943537104478240972== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4944/#review8021 ----------------------------------------------------------- src/common/utils.hpp Now that we're using ostringstream we should include 'sstream'. src/common/utils.hpp Why not call this mktemp? Also, spaces around '=3D' please. src/common/utils.hpp Wait, 'rootDir' is not a dir, it's a path, let's call it 'path'. src/common/utils.hpp Why not include strerror here? src/common/utils.hpp s/mktempdir/mkdtemp s/t=3D"/t =3D " src/common/utils.hpp Now it's a directory, but you might as well still call it path. ;) src/common/utils.hpp And strerror would again be helpful. src/common/utils.hpp s/reported/reported. src/common/utils.hpp const std::string& src/common/utils.hpp Formatting is wrong here. src/common/utils.hpp Good use of Result! src/common/utils.hpp Formatting. src/slave/path.hpp I think the suffix _PATH reads better to me. Likewise, I think either j= ust '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 Please reformat all opening '{' in this file. src/slave/path.hpp Why not call this enum 'Type'? Seems more descriptive than 'Data'. src/slave/path.hpp Wait, aren't there only two files? Do we really need a hashmap? src/slave/path.hpp Kill space. src/slave/path.hpp Again, aren't there only three files? Are these supposed to be the path= to these files? src/slave/path.hpp I think 'latest' is more descriptive than maxrun. I also think the clea= nest way to do this would really be, when someone wants the latest run: = RunState state; = int latest =3D sort(state.runs.keys()).back(); = But perhaps that's still not possible given our current libraries. *sig= h* src/slave/path.hpp Space after ',' please. src/slave/path.hpp 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 strin= gs. src/slave/path.hpp Are these duplicated in the .cpp? Either way, we shouldn't keep them he= re. src/slave/path.cpp Why not: = utils::os::glob(getPath(FRAMEWORK_LAYOUT, root, slaveId, '*')); = You can put that all on a newline. src/slave/path.cpp Formatting. src/slave/path.cpp This is great: the way this is being done "procedurally" now enables gr= eat log lines like this! src/slave/path.cpp 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 Formatting! src/slave/path.cpp For ma tt ing. src/slave/path.cpp Maybe just skip instead of fail? src/slave/path.cpp Seeing this done here like this makes me wonder why it can't be done la= ter, when it's needed. (For the others too.) src/slave/path.cpp "No tasks found for run " << run << " of executor " << executorId << " = of framework " << frameworkId src/slave/path.cpp Again, it seems like recording the TaskID is the most important part, a= nd getting the paths to these files can be done later, wherever it is neede= d. src/slave/path.cpp What about something like this here instead: = int run =3D 0; = Result > paths =3D glob(format(EXECUTOR_PATH, directory, i= d, frameworkId, executorId, '*')); = CHECK(!paths.isError()) << paths.error(); = if (paths.isSome()) { foreach (const string& path, paths.get()) { Try temp =3D numify(basename(path)); if (temp.isError()) { continue; } run =3D max(run, temp.get()); } } = string result =3D format(EXECUTOR_PATH, directory, id, frameworkId, exe= cutorId, run + 1); = bool created =3D mkdir(result); = CHECK(created) << ...; = return result; src/slave/path.cpp 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 Glad to see all of this go! It's been wreaking havoc on my ability to s= leep well at night. ;) src/tests/path_tests.cpp 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.or= g/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 > = > --===============0943537104478240972==--