> 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.

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


> On Aug. 26, 2015, 11:04 p.m., Alexander Rukletsov wrote:
> > include/mesos/mesos.proto, line 136
> > <https://reviews.apache.org/r/36571/diff/15/?file=1053336#file1053336line136>
> >
> >     Why do we need this message? I've checked several following patches and 
> > haven't seen any use cases. Shall I look further up review chain?

We use this later on when we add the `/maintenance/start` and 
`/maintenance/stop` endpoints, which both take a list of machines.
However, considering this https://reviews.apache.org/r/37826/ new review, it 
may not be necessary for much longer.


> On Aug. 26, 2015, 11:04 p.m., Alexander Rukletsov wrote:
> > src/Makefile.am, line 913
> > <https://reviews.apache.org/r/36571/diff/15/?file=1053337#file1053337line913>
> >
> >     Looks like we maintain alphabetical order here.

Good catch.  I think the `EXECUTOR_PROTO` and `ISOLATOR_PROTO` threw me off :)


> 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.

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.


- Joseph


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36571/#review96642
-----------------------------------------------------------


On Aug. 27, 2015, 11:58 a.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36571/
> -----------------------------------------------------------
> 
> (Updated Aug. 27, 2015, 11:58 a.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
> -------
> 
> * MachineInfo - Describes a single box that holds one or more agents.
> * MachineInfos - A list of boxes.
> * maintenance::Window - A set of machines and a planned downtime period.
> * maintenance::Schedule - A set of maintenance windows.
> * maintenance::Mode - An enum for the three states of maintenance: Normal, 
> Draining, Deactivated.
> * Registry::MaintenanceStatus - Holds the maintenance mode of a machine.
> 
> 
> Diffs
> -----
> 
>   include/mesos/maintenance/maintenance.hpp PRE-CREATION 
>   include/mesos/maintenance/maintenance.proto PRE-CREATION 
>   include/mesos/mesos.proto 33e1b28f1ccbe227657a14395f81df20e0a9e193 
>   src/Makefile.am 9fd71d1ddf442712977596e7a13969ff5c1d68db 
>   src/master/registry.proto a1995e56886f5296bc71d1bdfebec0d8316396c6 
> 
> Diff: https://reviews.apache.org/r/36571/diff/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Reply via email to