----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31362/#review73838 -----------------------------------------------------------
Looks great! I have a few minor nits and some questions. Once these are resolved/answered, I'll do another pass and (hopefully) give it a ShipIt. src/launcher/fetcher.cpp <https://reviews.apache.org/r/31362/#comment120245> 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? src/launcher/fetcher.cpp <https://reviews.apache.org/r/31362/#comment120246> We don't end log messages in punctuation. (Comments, however, must be punctuated.) src/launcher/fetcher.cpp <https://reviews.apache.org/r/31362/#comment120247> 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.. src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/31362/#comment120249> Maybe add a comment or a more explicit test for "There is no HADOOP_HOME" src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/31362/#comment120250> This wasn't necessary before, since you weren't using a relative path for URI, right? src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/31362/#comment120251> What non-0 return status do you expect? src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/31362/#comment120258> Should probably verify that 'proof' does not exist before the test runs. Maybe even delete it if it does somehow exist. src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/31362/#comment120256> Might you want to use '&&' instead of ';', in case the fake fetcher 'cp' fails? src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/31362/#comment120257> Very cool. Did you (manually) test that this actually works? - Adam B 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 > >
