----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12365/#review23410 -----------------------------------------------------------
Ship it! This looks like it still needs to get rebased on the other reviews, but this looks good to me. What is the current impact on the Java and Python examples until we provide Resources.java and resources.py? include/mesos/resources.hpp <https://reviews.apache.org/r/12365/#comment47292> The ramification of this is that we're forcing users to get/install stout on their own. Let's not worry about this right now, just wanted to bring it up for others and posterity. include/mesos/resources.hpp <https://reviews.apache.org/r/12365/#comment47291> Is this needed by the header? If so, we'll need to make this public too (which we should do anyway). include/mesos/resources.hpp <https://reviews.apache.org/r/12365/#comment47280> As a primitive, I'd prefer s/findWithRoles/find/. Also, we'll want to differentiate 'None' resources from an error. I'm not sure exactly what error we could have here? We might just want this to be 'Option<Resources> find(...)'. src/common/resources.cpp <https://reviews.apache.org/r/12365/#comment47293> This should not be an error. This should just be 'return None();'. src/examples/test_framework.cpp <https://reviews.apache.org/r/12365/#comment47282> static const Resources TASK_RESOURCES = Resources::parse( "cpus:" + stringify(CPUS_PER_TASK) + ";mem:" + stringify(MEM_PER_TASK)); src/examples/test_framework.cpp <https://reviews.apache.org/r/12365/#comment47283> s/taskResources/TASK_RESOURCES/ src/examples/test_framework.cpp <https://reviews.apache.org/r/12365/#comment47284> Option<Resources> resources = remaining.find(TASK_RESOURCES, role); CHECK_SOME(resources); task.mutable_resources()->MergeFrom(resources.get()); remaining -= resources; src/examples/test_framework.cpp <https://reviews.apache.org/r/12365/#comment47290> Let's use flags. ;) See src/tests/main.cpp for an example. It'll look something like: ---------------- Flags flags; string role; flags.add(&role, "role", "Role to use when registering", "*"); Option<string> master; flags.add(&master, "master", "ip:port of master to connect"); Try<Nothing> load = flags.load(None(), argc, argv); if (load.isError()) { cerr << load.error() << endl; usage(argv[0], flags); exit(1); } else if (master.isNone()) { cerr << "Missing --master" << endl; usage(argv[0], flags); exit(1); } ---------------- You can grab usage from src/tests/main.cpp. src/tests/mesos.hpp <https://reviews.apache.org/r/12365/#comment47289> See notes below, you can s/cpus_/cpus/ and s/mem_/mem/. src/tests/mesos.hpp <https://reviews.apache.org/r/12365/#comment47285> Same as above sans the 'static' part. ;) src/tests/mesos.hpp <https://reviews.apache.org/r/12365/#comment47286> Same as above. - Benjamin Hindman On July 16, 2013, 12:29 a.m., Thomas Marshall wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/12365/ > ----------------------------------------------------------- > > (Updated July 16, 2013, 12:29 a.m.) > > > Review request for mesos and Benjamin Hindman. > > > Bugs: MESOS-505 > https://issues.apache.org/jira/browse/MESOS-505 > > > Repository: mesos > > > Description > ------- > > One of the biggest potential problems with adding roles to resources is that > it significantly increases the complexity of dealing with resources for > scheduler writers - you end up in a situation where the Resources sent as > part of an offer may, for example, contain multiple cpu resources, each > marked with a different role, and you may want to launch tasks that use cpus > across roles. > > The solution for this is to provide the utilities of the Resources class for > use by scheduler writers. In particular, I've implemented a function called > findWithRoles that I believe covers the common case of launching tasks from > offers in a simple way - the idea is that a scheduler will construct a > Resources object representing the amount of resources it wants for a task but > not marked with roles. Then you call findWithRoles on the offer's resources > with the task's resources as the parameter. findWithRoles will return a > Resources object with the same total amount of resources as the parameter, > but with roles marked appropriately to match up with resources from the > offer, taking resources first from the role specified as a parameter to > findWithRoles, then from the default role, then from any other role. > test_framework.cpp is a good example of how this would work. > > > Diffs > ----- > > include/mesos/resources.hpp PRE-CREATION > src/Makefile.am 5d6f309 > src/common/resources.hpp 42dfb6a > src/common/resources.cpp 8bc1057 > src/common/type_utils.hpp 377b65f > src/common/values.cpp 36e11d1 > src/examples/test_framework.cpp 2c46c24 > src/master/allocator.hpp 9fc802a > src/master/drf_sorter.hpp 80f4798 > src/master/hierarchical_allocator_process.hpp c959c3f > src/master/http.cpp b5c8db9 > src/master/master.hpp a076658 > src/slave/http.cpp dc2955f > src/slave/isolator.hpp 67effd3 > src/slave/slave.hpp c679447 > src/tests/gc_tests.cpp 1745800 > src/tests/isolator_tests.cpp a37f006 > src/tests/mesos.hpp 5cb5aa2 > src/tests/resources_tests.cpp 67a11b3 > src/tests/slave_recovery_tests.cpp b0843d7 > src/tests/sorter_tests.cpp 619e786 > > Diff: https://reviews.apache.org/r/12365/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Thomas Marshall > >
