> On April 24, 2014, 7:09 p.m., Adam B wrote: > > src/launcher/fetcher.cpp, line 195 > > <https://reviews.apache.org/r/20674/diff/2/?file=567966#file567966line195> > > > > What if the [X|N] is left off the uri in MESOS_EXECUTOR_URIS (for those > > upgrading from old URI lists)? > > For substr, "If [pos] is equal to the string length, the function > > returns an empty string." and empty string != "X", so old URIs will not be > > extracted when they previously were.
I don't think that's an issue, because this is inside the external fetcher executable, which (from what I can tell) would only be called by the mesos containerizer. I find it unlikely that you might have a different version of the mesos-slave process running than that of the mesos-fetcher. You are indeed correct, however, that if someone is calling the fetcher themselves without passing [X|N] this will be reset (incorrectly) to false. > On April 24, 2014, 7:09 p.m., Adam B wrote: > > src/slave/containerizer/mesos_containerizer.cpp, line 68 > > <https://reviews.apache.org/r/20674/diff/2/?file=567967#file567967line68> > > > > uri.has_extract() && uri.extract() ? I originally implemented it like that, but to uphold the [default = true] option on the protobuf message, has_extract() will return false if the user doesn't specifically pass the value (older frameworks). This means if you don't specify a value, the "default" would in effect end up being false, which is not desired. > On April 24, 2014, 7:09 p.m., Adam B wrote: > > src/tests/containerizer_tests.cpp, lines 189-195 > > <https://reviews.apache.org/r/20674/diff/2/?file=567968#file567968line189> > > > > Can you also add a test for NoExtract if uri.set_extract is left off, > > since it's optional? Hmm, if you leave it off it's going to default to true, which is covered by the other tests cases in the file... I'm happy to add one if you'd rather it be more explicit. - Tom ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20674/#review41371 ----------------------------------------------------------- On April 24, 2014, 6:53 p.m., Tom Arnfeld wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20674/ > ----------------------------------------------------------- > > (Updated April 24, 2014, 6:53 p.m.) > > > Review request for mesos, Benjamin Hindman, Bernd Mathiske, Ian Downes, > Niklas Nielsen, and Vinod Kone. > > > Bugs: MESOS-1241 > https://issues.apache.org/jira/browse/MESOS-1241 > > > Repository: mesos-git > > > Description > ------- > > This is a patch that includes a new `extract` boolean value on > CommandInfo.URI to toggle the auto-extraction of compressed archives when > shipping URIs with an executor or task. > > It's worth noting that this changes the behaviour of the MESOS_EXECUTOR_URI > environment variable used. I've added some comments which outlines the > expected format, instead of (e.g) hdfs://foobar/testing.tar.gz+0 (1 > signifying it's not executable) there is now an extra flag on the end (X or > N) defining whether to auto-extract or not, resulting in > hdfs://foobar/testing.tar.gz+0X by default. > > > Diffs > ----- > > include/mesos/mesos.proto 37f8a7f > src/launcher/fetcher.cpp 98ebc2e > src/slave/containerizer/mesos_containerizer.cpp 722f3fe > src/tests/containerizer_tests.cpp 6c48146 > > Diff: https://reviews.apache.org/r/20674/diff/ > > > Testing > ------- > > - Tested with a single-slave mesos cluster (Mac OSX 10.9) > - Verified `make check` after adding extra unit tests > > > Thanks, > > Tom Arnfeld > >
