mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrei Sekretenko <asekrete...@apache.org>
Subject Re: Review Request 72955: Consolidated creation and validation of `allocator::Framework` options.
Date Wed, 14 Oct 2020 18:30:56 GMT


> On Oct. 14, 2020, 12:54 a.m., Benjamin Mahler wrote:
> > src/master/master.hpp
> > Lines 1061-1062 (original), 1061-1062 (patched)
> > <https://reviews.apache.org/r/72955/diff/1/?file=2240632#file2240632line1061>
> >
> >     Maybe just say "based on master flags and the framework not being completed"?
> >     
> >     Also, this makes it sound like it only validates things against the master flags
and the framework being non-completed, but it also validates somewhat that the FrameworkInfo
is valid on its own, like the failover timeout. Should the failover timeout validation be
moved to the existing stateless validation of FrameworkInfo?

Moving the failover timeout into stateless validation is a right thing, sent a patch for that.
Nevertheless, the very first thing this function does is calling the stateless validation,
and I have no intention to change this now.

I've reworded this comment change to avoid giving impression that stateless validation is
performed separately.


> On Oct. 14, 2020, 12:54 a.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 2588-2598 (original)
> > <https://reviews.apache.org/r/72955/diff/1/?file=2240633#file2240633line2589>
> >
> >     Per above comment, did you mean to also move out the failover timeout validation,
which looks to be a similarly stateless validation of the FrameworkInfo as this bit?
> 
> Andrei Sekretenko wrote:
>     Thanks! I somehow missed that it is entirely stateless. Will send one more small
patch.

Patch for moving failover timeout validation: https://reviews.apache.org/r/72964/


> On Oct. 14, 2020, 12:54 a.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 2629-2634 (patched)
> > <https://reviews.apache.org/r/72955/diff/1/?file=2240633#file2240633line2641>
> >
> >     Hm.. shouldn't this stateless FrameworkInfo validation go in the existing  `validation::framework::valdiate(const
FrameworkInfo&)` function in master/validation.hpp?

I would say the this validates not `FrameworkInfo` but rather suppressed roles, which we pass
separately (and do not get from resubscribing agents, only via scheduler API).

Now this patch moves this validation into a separated function in `validation.hpp`; not sure
if it makes things better or worse, though.


- Andrei


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


On Oct. 14, 2020, 8:16 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72955/
> -----------------------------------------------------------
> 
> (Updated Oct. 14, 2020, 8:16 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-10176
>     https://issues.apache.org/jira/browse/MESOS-10176
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This merges three near-identical pieces of scattered code in SUBSCRIBE
> and UPDATE_FRAMEWORK execution paths in the Master that validate
> and construct parts of `allocator::FrameworkOptions` (the set of
> suppressed roles and the offer constraints filter) into a single
> function.
> 
> This is a prerequisite to adding validation of offer constraint roles.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 83d8190702ff25b5e3e9607d8250aa331aaad028 
>   src/master/master.cpp d6d3ea7e27539a65b601783eca2ac4e3b688f188 
>   src/master/validation.hpp 7fe8f087b322f8e77218f4dd9f9f75707afa3208 
>   src/master/validation.cpp 5b1bcb5207aac0553bf9c1a0d89525c835393ccb 
> 
> 
> Diff: https://reviews.apache.org/r/72955/diff/3/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>


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