> On Aug. 26, 2015, 11:04 p.m., Alexander Rukletsov wrote: > > include/mesos/maintenance/maintenance.proto, lines 31-37 > > <https://reviews.apache.org/r/36571/diff/15/?file=1053335#file1053335line31> > > > > IIUC, here you group a bunch of machines based on time intervals. Why > > this grouping is necessary? We can simplify the design significantly and > > reduce the number of protobufs if we represent schedule by a set of > > maintenance events, each per machine, which may occasionally overlap in > > time. > > > > See my comment in https://reviews.apache.org/r/37314 with a proposal > > for a joined "maintenance event" message. > > Joseph Wu wrote: > We should definitely discuss this further. > > The motivation behind this "schedule" and "window" representation is that > operators generally think in these terms too. Since these protos are what > the operators are expected to use to interact with maintenance primitives, > we'd like it to match their understanding. > > Alexander Rukletsov wrote: > Absolutely, we should not surprise operators. But they do not speak > protobufs, they will be sending JSON requests, which we may convert to > something (protobufs) that is most convenient for us in terms of storage and > retrieval.
I think a 1-to-1 protobuf-JSON symmetry is useful for documenting both JSON and protobuf at once. We'll probably keep this how it is. However, we'll be using your suggested "joined message". > On Aug. 26, 2015, 11:04 p.m., Alexander Rukletsov wrote: > > include/mesos/maintenance/maintenance.proto, line 56 > > <https://reviews.apache.org/r/36571/diff/15/?file=1053335#file1053335line56> > > > > I'm not sure `0` is a valid protobuf tag. > > Joseph Wu wrote: > After double checking the docs... It is valid for enums: > https://developers.google.com/protocol-buffers/docs/proto#enum (But not valid > for non-enums.) > > For example: > https://github.com/apache/mesos/blob/master/include/mesos/mesos.proto#L434 > > Vinod Kone wrote: > I would strongly recommend to use a start value of 1 instead of 0 because > it's hard to tell the difference between an enum being unset to enum being > set to the first value. This is especially true if the enum is going to be > used as optional. Looks like it is used as a required field for now, but who > knows. Good point. I'll increment by one. - Joseph ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36571/#review96642 ----------------------------------------------------------- On Aug. 27, 2015, 4:41 p.m., Joseph Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36571/ > ----------------------------------------------------------- > > (Updated Aug. 27, 2015, 4:41 p.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, > Joris Van Remoortere, and Vinod Kone. > > > Bugs: MESOS-3066 > https://issues.apache.org/jira/browse/MESOS-3066 > > > Repository: mesos > > > Description > ------- > > New protobufs: > > * MachineAddress - Describes a single box that holds one or more agents. > * MachineAddresses - A list of boxes. > * MachineInfo - Holds a box and some extra information about its status. > * MachineInfo::Mode - An enum for the three states of a machine: UP, > DRAINING, DOWN. > * maintenance::Window - A set of machines and a planned downtime period. > * maintenance::Schedule - A set of maintenance windows. > > > Diffs > ----- > > include/mesos/maintenance/maintenance.hpp PRE-CREATION > include/mesos/maintenance/maintenance.proto PRE-CREATION > include/mesos/mesos.proto 715b8cf38e1e56c18a3f2ddbb82c920bd9414f05 > include/mesos/v1/mesos.proto 382b978dca769757171c5558b7f259870592c321 > src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 > src/master/registry.proto a1995e56886f5296bc71d1bdfebec0d8316396c6 > > Diff: https://reviews.apache.org/r/36571/diff/ > > > Testing > ------- > > `make check` > > > Thanks, > > Joseph Wu > >
