> 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. > > Tom Arnfeld wrote: > 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.
I think it's okay to assume that if an operator reinstalls the slave then they'll get an updated fetcher too, so I think we can drop this issue. > 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? > > Tom Arnfeld wrote: > 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. Yup, the rest of the tests are testing it without 'extract' which should be sufficient. > 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() ? > > Tom Arnfeld wrote: > 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. Yes, we always want to get 'extract' since it's default value is set to true so we don't need to call 'has_extract' (or if we did we'd have to assume that not having 'extract' actually implies that we should do the extraction). - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20674/#review41371 ----------------------------------------------------------- On April 25, 2014, 6:28 a.m., Tom Arnfeld wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20674/ > ----------------------------------------------------------- > > (Updated April 25, 2014, 6:28 a.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 d7fe068 > src/launcher/fetcher.cpp 98ebc2e > src/slave/containerizer/mesos_containerizer.cpp f5df979 > 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 > >
