mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrei Sekretenko <asekrete...@mesosphere.io>
Subject Re: Review Request 72786: Implemented regex-based offer constraints.
Date Thu, 27 Aug 2020 14:04:43 GMT


> On Aug. 26, 2020, 11:28 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/offer_constraints_filter.hpp
> > Lines 27 (patched)
> > <https://reviews.apache.org/r/72786/diff/7/?file=2238780#file2238780line27>
> >
> >     Hm.. why is this decoupled into a different header from OfferConstraintsFilter?
Is it to minimize the stuff pulled into master.hpp? If so, not sure if that's worth the extra
indirection?

Right at this moment, this also helps minimize things that are kept in the public header `<mesos/allocator/allocator.hpp>`
(which, for example, is included into something like 1/3 of the volume of our tests, directly
or indirectly).


> On Aug. 26, 2020, 11:28 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/offer_constraints_filter.cpp
> > Lines 68 (patched)
> > <https://reviews.apache.org/r/72786/diff/7/?file=2238781#file2238781line68>
> >
> >     No period at the end
> >     
> >     Can we include the program size value and max?
> >     
> >     E.g.
> >     
> >     Regex 'foo|bar' is too complex: program size of 112 vs maximum of 100 allowed
via --offer_constraints_re2_max_program_size

The value and the limit definitely make sense! Added both.

Not sure if we want to add one more location that contains the non-local knowledge about how
exactly the limit is configured.
I would hope that if a regex breaches the limit, the author of the regex in most cases will
be able to reformulate the constraints instead of pushing the cluster administrator to increase
the global limit...


> On Aug. 26, 2020, 11:28 p.m., Benjamin Mahler wrote:
> > src/tests/master/offer_constraints_filter_tests.cpp
> > Lines 46 (patched)
> > <https://reviews.apache.org/r/72786/diff/7/?file=2238787#file2238787line46>
> >
> >     Should we have the type of this also be Bytes, or clarify the units in the name?
> >     
> >     ```
> >       options.re2Limits.maxMem = Kilobytes(4);
> >     ```
> >     
> >     ```
> >       options.re2Limits.maxMemInBytes = 4 * 1024;
> >     ```

Thanks, looks like I forgot to adjust this location; the first option is clearly better.

Makes me wonder if the `Bytes(unit64_t)` constructor should be made `explicit`...


- Andrei


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


On Aug. 27, 2020, 11:52 a.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72786/
> -----------------------------------------------------------
> 
> (Updated Aug. 27, 2020, 11:52 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-10173
>     https://issues.apache.org/jira/browse/MESOS-10173
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented regex-based offer constraints.
> 
> 
> Diffs
> -----
> 
>   include/mesos/allocator/allocator.hpp 05d0e9c46485c3048a65d46747d56e715883a0b3 
>   src/Makefile.am 8b95611baff2f925910ea46addf462cf9f33071b 
>   src/master/allocator/mesos/offer_constraints_filter.hpp PRE-CREATION 
>   src/master/allocator/mesos/offer_constraints_filter.cpp d724fd0a9832feea26f6db1a498b6bdee83ffff0

>   src/master/constants.hpp c384b6878193a4b6bb09294d8b14757906595c1b 
>   src/master/flags.hpp 78623d68bf428cd3f52684303d98a525d42eb878 
>   src/master/flags.cpp 74f4daadd48e8e691be43759b88dc8b3c2df489a 
>   src/master/master.hpp e478f805069813532011f2a1991ab1f847080516 
>   src/master/master.cpp 4428985902512eea1c97205f04a4fed6c16d494e 
>   src/tests/master/offer_constraints_filter_tests.cpp 84a1e917a52d0b98f979e454c2b982c6402b0176

> 
> 
> Diff: https://reviews.apache.org/r/72786/diff/9/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>


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