Return-Path: X-Original-To: apmail-mesos-dev-archive@www.apache.org Delivered-To: apmail-mesos-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 2557B10C19 for ; Wed, 25 Feb 2015 07:00:27 +0000 (UTC) Received: (qmail 35839 invoked by uid 500); 25 Feb 2015 07:00:27 -0000 Delivered-To: apmail-mesos-dev-archive@mesos.apache.org Received: (qmail 35764 invoked by uid 500); 25 Feb 2015 07:00:26 -0000 Mailing-List: contact dev-help@mesos.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@mesos.apache.org Delivered-To: mailing list dev@mesos.apache.org Received: (qmail 35745 invoked by uid 99); 25 Feb 2015 07:00:26 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 25 Feb 2015 07:00:26 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 779171D319B; Wed, 25 Feb 2015 07:00:24 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============4877984977250728919==" MIME-Version: 1.0 Subject: Re: Review Request 31362: Reenabled hadoop_home and frameworks_home slave flags for mesos-fetcher and added tests From: "Bernd Mathiske" To: "Niklas Nielsen" , "Adam B" , "Ben Mahler" Cc: "Bernd Mathiske" , "mesos" Date: Wed, 25 Feb 2015 07:00:24 -0000 Message-ID: <20150225070024.4175.21607@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: "Bernd Mathiske" X-ReviewGroup: mesos X-ReviewRequest-URL: https://reviews.apache.org/r/31362/ X-Sender: "Bernd Mathiske" References: <20150224182552.4175.57992@reviews.apache.org> In-Reply-To: <20150224182552.4175.57992@reviews.apache.org> Reply-To: "Bernd Mathiske" X-ReviewRequest-Repository: mesos --===============4877984977250728919== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit > On Feb. 24, 2015, 10:25 a.m., Adam B wrote: > > src/launcher/fetcher.cpp, lines 172-173 > > > > > > What if the user had set the MESOS_FRAMEWORKS_HOME env variable instead of the --frameworks_home flag? Would that have still worked in 0.21? Will it work now? MESOS_FRAMEWORKS_HOME is not an env var that anybody else than mesos-fetcher itself was expected to use for anything. It is supposed to be brought into the fold with all other MESOS_XXX env vars that disappeared into the JSON object that now represents fetcher parameters. Sorry that this was not done earlier! HADOOP_HOME is different, because it has significance outside internal Mesos affairs. So we are keeping this one. > On Feb. 24, 2015, 10:25 a.m., Adam B wrote: > > src/launcher/fetcher.cpp, line 180 > > > > > > We don't end log messages in punctuation. (Comments, however, must be punctuated.) I will comply even though in this case we have two complete sentences. Would you like me to remove the period after the first sentence, two? Hm. I'll put a semicolon there to side-step this issue :-) > On Feb. 24, 2015, 10:25 a.m., Adam B wrote: > > src/launcher/fetcher.cpp, lines 300-303 > > > > > > if fetch() takes an Option already, can we just pass it `fetcherInfo.get().frameworks_home()` directly? I thought there might be some auto-translation between optional protobufs and Options.. There is no such auto-translation that I am aware of. Please point me to the routine that does this! Just passing directly could AFAIK result in a SOME with an empty string instead of a NONE. See fetcher.pb.h: inline const ::std::string& FetcherInfo::frameworks_home() const { return *frameworks_home_; } > On Feb. 24, 2015, 10:25 a.m., Adam B wrote: > > src/tests/fetcher_tests.cpp, line 343 > > > > > > This wasn't necessary before, since you weren't using a relative path for URI, right? Right. > On Feb. 24, 2015, 10:25 a.m., Adam B wrote: > > src/tests/fetcher_tests.cpp, line 435 > > > > > > What non-0 return status do you expect? Our Subprocess (see subprocess.hpp) implementation does not report the exit code from the program it ran. Instead it returns what waitpid() reports, which is a child process ID in this case. This is different every time. I think this behavior of Subprocess is less useful than it could be, but that's a separate issue. > On Feb. 24, 2015, 10:25 a.m., Adam B wrote: > > src/tests/fetcher_tests.cpp, line 734 > > > > > > Should probably verify that 'proof' does not exist before the test runs. Maybe even delete it if it does somehow exist. This is a path extension from os::getcwd() in a freshly created sandbox: class FetcherTest : public TemporaryDirectoryTest {}; ... TEST_F(FetcherTest, HdfsURI) { ... > On Feb. 24, 2015, 10:25 a.m., Adam B wrote: > > src/tests/fetcher_tests.cpp, line 737 > > > > > > Might you want to use '&&' instead of ';', in case the fake fetcher 'cp' fails? Not in this case. I see that fetcher cp fails if the file is not there. Then, debugging this program, seeing "proof" tells me that the script was invoked. Otherwise I might be fooled into believing it wasn't. > On Feb. 24, 2015, 10:25 a.m., Adam B wrote: > > src/tests/fetcher_tests.cpp, line 763 > > > > > > Very cool. Did you (manually) test that this actually works? This is copied from other tests in this file. BenH wrote this. I deleted it in an upcoming patch to reduce code volume. Let me know if you desperately need it back when looking at r30774... Yes, this works. - Bernd ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31362/#review73838 ----------------------------------------------------------- On Feb. 24, 2015, 9:28 a.m., Bernd Mathiske wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31362/ > ----------------------------------------------------------- > > (Updated Feb. 24, 2015, 9:28 a.m.) > > > Review request for mesos, Adam B, Ben Mahler, and Niklas Nielsen. > > > Bugs: MESOS-2390 > https://issues.apache.org/jira/browse/MESOS-2390 > > > Repository: mesos > > > Description > ------- > > Now the containerizer/fetcher sets the HADOOP_HOME variable for mesos-fetcher again if the slave flag hadoop_home is set. Added a test that checks that HDFS fetching is not broken and also ensures that the flag gets translated to the environment variable and then gets applied in mesos-fetcher. Created a mock hadoop implementation script for this. This script has the exact same side effects as a real haddop client in the scope of our testing. Using this, Mesos testing has no extra external dependencies (on Hadoop). > > Slave flag frameworks_home does not need to be an evironment variable. It is now part of FetcherInfo only, but it also gets tested now that it works. > > > Diffs > ----- > > include/mesos/fetcher/fetcher.proto facb87b92bf3194516f636dcc348e136af537721 > src/launcher/fetcher.cpp fed0105946da579a38357a30e7ae56e646e05b89 > src/slave/containerizer/fetcher.cpp d290f95251def3952c5ee34f600e1d71467f6293 > src/tests/fetcher_tests.cpp 2438620e2db94c3c56336fa5d8e69a18fe8f3bac > src/tests/mock_hadoop.sh PRE-CREATION > > Diff: https://reviews.apache.org/r/31362/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Bernd Mathiske > > --===============4877984977250728919==--