----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30774/#review71575 -----------------------------------------------------------
src/launcher/fetcher.cpp <https://reviews.apache.org/r/30774/#comment117341> Separate with newline please. src/launcher/fetcher.cpp <https://reviews.apache.org/r/30774/#comment117342> '{' on previous line please. src/launcher/fetcher.cpp <https://reviews.apache.org/r/30774/#comment117343> Kill '} else {'. src/slave/containerizer/fetcher.hpp <https://reviews.apache.org/r/30774/#comment117344> Alphabetical ordering please (move 'stout' down below 'process'). src/slave/containerizer/fetcher.hpp <https://reviews.apache.org/r/30774/#comment117345> If we change the name here we should change the name in the Fetch class too please src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30774/#comment117347> This error message is a bit little wierd since it's not that it can't be removed, it's that we can determine it's 'realpath'. Also, why not use that for the actual os::rmdir below? src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30774/#comment117346> Fix indentation. src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30774/#comment117349> auto downloads = make_shared<hashmap<string, Bytes>>(); Also, I think using 'auto' here makes sense because the type information is redundant. Let's send an email to the mailing list with this as a proposal to use 'auto' in these contexts because the type information is redundant, where as the type information when we get something returned from a function is invaluable! src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30774/#comment117348> Were you explicitly trying to do this asynchronously? Since it could be an expensive operation (filesystem) I'm totally for it but just wanted to confirm. src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30774/#comment117350> Let's make HDFS::du() do more work for future people using it by moving this code into HDFS::du and having that return a Try<Bytes>. src/tests/fetcher_cache_tests.cpp <https://reviews.apache.org/r/30774/#comment117361> Is this supported by gcc 4.4? That's still the required compiler until after 0.22 is cut. How about we just make these be globals for now. same for the ones below too. src/tests/fetcher_cache_tests.cpp <https://reviews.apache.org/r/30774/#comment117362> CHECK_SOME(os::mkdir(...)); src/tests/fetcher_cache_tests.cpp <https://reviews.apache.org/r/30774/#comment117363> We just name the enum directly rather than with the typedef: enum ExecutionMode { ... }; src/tests/fetcher_cache_tests.cpp <https://reviews.apache.org/r/30774/#comment117370> There is only "checkpointing" mode now, so don't bother trying to test both cases. src/tests/fetcher_cache_tests.cpp <https://reviews.apache.org/r/30774/#comment117371> Please structure the test using the 'SetUp' 'TearDown' method overloads rather than handrolling this stuff yourself; it's far easier to follow when using 'SetUp' and 'TearDown' and they let you embed ASSERT_* and EXPECT_* cleanly. Note that when you need a slightly different setup/teardown flow then we either parameterize the test case or create more subclasses (e.g., subclasses of FetcherCacheTest such as ArchiveAssetFetcherCacheTest and ExecutableAssetFetcherCacheTest). src/tests/fetcher_cache_tests.cpp <https://reviews.apache.org/r/30774/#comment117364> '{' on newline please. src/tests/fetcher_cache_tests.cpp <https://reviews.apache.org/r/30774/#comment117372> This should occur in the HttpServer but then you should have an HttpServerProcess. When does process terminate? How are you sure that libprocess is not calling something like 'visit' after the memory for this class has been cleaned up!!! src/tests/fetcher_cache_tests.cpp <https://reviews.apache.org/r/30774/#comment117365> s/node/address/ src/tests/fetcher_cache_tests.cpp <https://reviews.apache.org/r/30774/#comment117373> I don't like the mixture of actors and threads here. Even if this is only in the tests, it sets a bad precedent that we're willing compromise on the integrity of the assumptions you have when executing within an actor. There is also a lot of expectations shared between the implementation here and the tests themselves. For example, 'pause' is meant to be used in order to enable queuing up multiple pending cache entry downloads. The abstraction I think you want here is something that lets you control what happens whenever we get a 'visit' callback, and to do that we really need to mock 'visit'. If you had a mocked 'visit' then you can explicitly control "how", and most importantly, "when" each 'visit' call occurs. src/tests/fetcher_cache_tests.cpp <https://reviews.apache.org/r/30774/#comment117379> We've got a concurrent queue in libprocess called 'Queue'. See scheduler_tests.cpp for how it's used. I'd love to see the 'Enqueue' gmock action be made more generic so that you can use it here too (I'm confident it can, and can help if need be, and would rather use that then introduce another concurrent queuing like data structure, even if it's only for the tests). src/tests/fetcher_cache_tests.cpp <https://reviews.apache.org/r/30774/#comment117374> Why aren't we being more explicit about what we're checking for here? src/tests/fetcher_cache_tests.cpp <https://reviews.apache.org/r/30774/#comment117377> There is a ton of state, expectations, control flow requirements, etc, throughout these tests. This makes them very hard to follow, and thus hard to understand if you're really testing the things you want to test (i.e., concurrent requests, etc). While it's probably more code, I'd rather have seen each test case testing exactly what you wanted to test rather than a bunch of functions pulled out to cover "commonality". It makes it very difficult for me to determine if certain test situations are covered! For example, in this function you await tasks if in "concurrent" mode. Yet, I'm pretty sure tasks aren't going to run in parallel unless there are enough resources to do so, and tracking down the size of the tasks and how many resources were launched with the slave is so non-local that it requires looking all over this source file and after doing so I'm not convinced that anything can actually run in parallel since the default slave has 2 CPUs and each task takes 1 CPU (so maybe 2 tasks run in parallel but not all of them?). Why not be more explicit in the tests, even if it's more code, so that we're convinced we're testing the things we want to test? My guess is that there will still be things we can pull out and abstract after having done that. src/tests/fetcher_cache_tests.cpp <https://reviews.apache.org/r/30774/#comment117378> How do you know that this index exists? Shouldn't we be doing a check before we loop through to make sure? - Benjamin Hindman On Feb. 8, 2015, 7:34 p.m., Bernd Mathiske wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30774/ > ----------------------------------------------------------- > > (Updated Feb. 8, 2015, 7:34 p.m.) > > > Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and > Timothy Chen. > > > Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, and MESOS-2074 > https://issues.apache.org/jira/browse/MESOS-2057 > https://issues.apache.org/jira/browse/MESOS-2069 > https://issues.apache.org/jira/browse/MESOS-2070 > https://issues.apache.org/jira/browse/MESOS-2072 > https://issues.apache.org/jira/browse/MESOS-2074 > > > Repository: mesos > > > Description > ------- > > Replaces all other reviews for the fetcher cache except those related to > stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, > 30618, 30621, 30626. See descriptions of those. > > > Diffs > ----- > > include/mesos/fetcher/fetcher.proto > facb87b92bf3194516f636dcc348e136af537721 > include/mesos/mesos.proto 3a2921dff856d37455593dcbf7340aa537997d35 > src/Makefile.am 0fe4809542a7d23785a221901947771320b43d52 > src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b > src/launcher/fetcher.cpp 5b2d86d1867b25bf71461fd73df91b089002325b > src/slave/constants.hpp 7717abd4f9a3bd6fdca6af2364864e374ce2e056 > src/slave/containerizer/docker.hpp 70114dcef7a416ae904e95eb9ce365bcd866aebc > src/slave/containerizer/docker.cpp 813ddf8c98622748b2da1b739bf122387abc4c79 > src/slave/containerizer/fetcher.hpp > bfd98dbe16e2bd5df3e2c8e9b10e303654f33446 > src/slave/containerizer/fetcher.cpp > 6e6bce08d76bb8a5813c905e3ffeff9b2411fd6d > src/slave/containerizer/mesos/containerizer.hpp > b3aafe4efa9f3469d7a3fd39243ad66b46d6a54d > src/slave/containerizer/mesos/containerizer.cpp > fa40d47aee7803833bcde6cce1e86a21d7cf27d0 > src/slave/flags.hpp f6033355d129f0013d39dd053455c936596bf159 > src/slave/slave.cpp fff2d725fe49eee984d9151cfb2131202c47994f > src/tests/docker_containerizer_tests.cpp > 8b212d4e6ed5a179ebadce1bdbbf3edde87d706c > src/tests/fetcher_cache_tests.cpp PRE-CREATION > src/tests/fetcher_tests.cpp fcbf7ad912f0b1b3d106a75a9dcb8213a0c69c07 > src/tests/mesos.hpp 83a369968ab2403fa341829ac5d11f7243095190 > src/tests/mesos.cpp 21a405366f56c963611324076efe775f85b9d9f7 > src/tests/mock_hadoop.sh PRE-CREATION > > Diff: https://reviews.apache.org/r/30774/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Bernd Mathiske > >
