> On June 13, 2013, 12:50 a.m., Benjamin Hindman wrote: > > This looks great except for some tests for parse.
Tests? Parsing really demands tests. There are always corner cases. In this case, you should definitely test the "only one role can have no resources" case. > On June 13, 2013, 12:50 a.m., Benjamin Hindman wrote: > > src/common/dedications.cpp, lines 49-50 > > <https://reviews.apache.org/r/11720/diff/1/?file=301977#file301977line49> > > > > Formatting. The formatting still looks incorrect here. > On June 13, 2013, 12:50 a.m., Benjamin Hindman wrote: > > src/common/dedications.cpp, lines 54-55 > > <https://reviews.apache.org/r/11720/diff/1/?file=301977#file301977line54> > > > > Formatting. And the formatting still looks incorrect here too. > On June 13, 2013, 12:50 a.m., Benjamin Hindman wrote: > > src/slave/flags.hpp, line 54 > > <https://reviews.apache.org/r/11720/diff/1/?file=301978#file301978line54> > > > > I never liked the delimiters I picked for resources, and this is > > getting pretty complicated too. What about using JSON to describe these > > things instead? For example: > > > > --resources='{ "cpus": 24.0, "disks": [disk1, disk2], "ports": > > "[3000-4000]", ... }' > > > > --reservations='{ "role1": { "cpus": 14.0, "disks": [disk1], "ports": > > "[3000-3500]", ... }, "role2": { "cpus": 10.0, "disks": [disk2], "ports": > > "[3501-4000]", ...}}' > > > > The caveat here is that we'd have to represent "ranges" as a string > > since you can't natively represent that in JSON without an object (IIUC). > > > > Thoughts? We should punt this for now anyway since we don't have a JSON > > parsing library. But it probably makes sense to clean this up for 1.0 and > > we might even be able to get to it for 0.14.0. Want to create an issue? > > > > Brenden Matthews wrote: > Another idea would be to use the protobuf text format. Either way, the > current resource format isn't great, which is why I needed to do this: > https://reviews.apache.org/r/11487/ > > Thomas Marshall wrote: > I definitely agree we should clean this up. An advantage to the protobuf > text format is that we already have the libraries we would need in place. > > Either way, issue created: https://issues.apache.org/jira/browse/MESOS-507 Thanks for creating the issue! - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11720/#review21826 ----------------------------------------------------------- On June 14, 2013, 1:35 a.m., Thomas Marshall wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/11720/ > ----------------------------------------------------------- > > (Updated June 14, 2013, 1:35 a.m.) > > > Review request for mesos and Benjamin Hindman. > > > Description > ------- > > This patch adds a Dedication protobuf, Dedications field to SlaveInfo, > Dedications class with a parse function, and a dedication command line flag > to the slave. > > > This addresses bug MESOS-505. > https://issues.apache.org/jira/browse/MESOS-505 > > > Diffs > ----- > > include/mesos/mesos.proto 8cbcd9a > src/Makefile.am e67b342 > src/common/reservations.hpp PRE-CREATION > src/common/reservations.cpp PRE-CREATION > src/slave/flags.hpp 9612983 > src/slave/slave.hpp d1ba82e > src/slave/slave.cpp b5b7e0e > > Diff: https://reviews.apache.org/r/11720/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Thomas Marshall > >
