mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bernd Mathiske" <be...@mesosphere.io>
Subject Re: Review Request 31362: Reenabled hadoop_home and frameworks_home slave flags for mesos-fetcher and added tests
Date Wed, 25 Feb 2015 07:00:24 GMT


> On Feb. 24, 2015, 10:25 a.m., Adam B wrote:
> > src/launcher/fetcher.cpp, lines 172-173
> > <https://reviews.apache.org/r/31362/diff/1/?file=874122#file874122line172>
> >
> >     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
> > <https://reviews.apache.org/r/31362/diff/1/?file=874122#file874122line180>
> >
> >     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
> > <https://reviews.apache.org/r/31362/diff/1/?file=874122#file874122line300>
> >
> >     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
> > <https://reviews.apache.org/r/31362/diff/1/?file=874124#file874124line343>
> >
> >     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
> > <https://reviews.apache.org/r/31362/diff/1/?file=874124#file874124line435>
> >
> >     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
> > <https://reviews.apache.org/r/31362/diff/1/?file=874124#file874124line734>
> >
> >     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
> > <https://reviews.apache.org/r/31362/diff/1/?file=874124#file874124line737>
> >
> >     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
> > <https://reviews.apache.org/r/31362/diff/1/?file=874124#file874124line763>
> >
> >     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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message