mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vinod Kone <vinodk...@apache.org>
Subject Re: Review Request 70822: Added common protobufs for agent draining.
Date Tue, 18 Jun 2019 23:45:02 GMT


> On June 14, 2019, 9:34 a.m., Benjamin Bannier wrote:
> > src/messages/messages.proto
> > Lines 986 (patched)
> > <https://reviews.apache.org/r/70822/diff/3/?file=2148591#file2148591line986>
> >
> >     What does it mean if the master sends a `DRAINED` state to the agent? Is that
something we need to reject in validation?
> >     
> >     Does it maybe make sense to instead break out `DrainInfo.state` into its own
message to use in state reporting?
> 
> Greg Mann wrote:
>     Yes we can have a CHECK on the agent to make sure the master doesn't send DRAINED
in a DrainSlaveMessage. That will happen in another patch.
>     
>     What's the benefit of moving `DrainInfo.State` outside of `DrainInfo`?
> 
> Benjamin Bannier wrote:
>     The goal would be to use dedicated messages for triggering draining (master->agent)
and to report draining (agent->master). That might not only be conceptually simplier, but
would likely also lead to simpler, less redundant code. Is there any benefit in using the
same message?
> 
> Greg Mann wrote:
>     What do you mean by "report draining"? The design doesn't involve any explicit communication
of draining progress between agent and master, the master just receives task status updates.
> 
> Benjamin Bannier wrote:
>     Sorry, there's no process of communicating `DrainInfo` back to master, but we do
report it to users with https://reviews.apache.org/r/70835/.
> 
> Greg Mann wrote:
>     Are you proposing having a separate message for inclusion in the agent API outputs?

IIUC, Benjamin is questioning why we are storing both the spec (max grace period, mark_gone)
and the status (state) in the same proto (do we do this elsewhere in the code base?). If they
were separate protos, say DrainRequest and DrainStatus, master would send DrainRequest to
the agent, and show DrainStatus (and possibly DrainRequest) in the operator API response.
It looks a bit weird that we would send DrainInfo (as it is currently written  to the agent
and do a CHECK on valid state. The fact that an agent got a Drain message is enough for the
agent to know that it needs to drain, it doesn't need to look into the `DrainInfo::State`?


- Vinod


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


On June 18, 2019, 11:17 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70822/
> -----------------------------------------------------------
> 
> (Updated June 18, 2019, 11:17 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benno Evers, Benjamin Mahler, Joseph Wu,
and Vinod Kone.
> 
> 
> Bugs: MESOS-9753
>     https://issues.apache.org/jira/browse/MESOS-9753
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch makes protobuf message updates which will be used
> by both the master and the agent to facilitate automatic
> draining of agents.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 2b4f350815935220c2d2b0dd0e52346bc74c91d9 
>   include/mesos/type_utils.hpp 57b1893160dbe874aa9fec00a3d1b640b9c54906 
>   include/mesos/v1/mesos.proto bafc27499f810791700c4a30dcb1da33b6f31d2e 
>   src/common/type_utils.cpp ef1b3ea15cde1c7a8e0735fb9d7566dd1fd2cfdb 
>   src/internal/devolve.hpp fefe86e450fa5083b9ff50e92f4594ffb30a54c8 
>   src/internal/devolve.cpp 1d300b49d5cc3de4b8ed409902eb881c7afc07ea 
>   src/internal/evolve.hpp 1044d9df75b6fc1f60d3704be9cb5751e6d4321d 
>   src/internal/evolve.cpp 19c155967bf090fb2ec39211805ff1385787ab59 
>   src/messages/messages.proto e30ad34cc9212b05f85ba5e1d4fcfc9e49ae92c0 
> 
> 
> Diff: https://reviews.apache.org/r/70822/diff/5/
> 
> 
> Testing
> -------
> 
> `make`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message