Re: Review Request 72786: Implemented regex-based offer constraints.

[Andrei Sekretenko]

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

(Updated Aug. 28, 2020, 12:41 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Moved the `OfferConstraintsFilter::Options` definition into `allocator.hpp`.

Left TODO for considering decoupling the filter creation interface in case it 
doesn't eventually get decoupled for other reasons.


Bugs: MESOS-10173
https://issues.apache.org/jira/browse/MESOS-10173


Repository: mesos


Description
---

Implemented regex-based offer constraints.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
05d0e9c46485c3048a65d46747d56e715883a0b3 
  src/Makefile.am 8b95611baff2f925910ea46addf462cf9f33071b 
  src/master/allocator/mesos/offer_constraints_filter.cpp 
d724fd0a9832feea26f6db1a498b6bdee830 
  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/10/

Changes: https://reviews.apache.org/r/72786/diff/9-10/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 72786: Implemented regex-based offer constraints.

[Andrei Sekretenko]

> On Aug. 26, 2020, 11:28 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/offer_constraints_filter.hpp
> > Lines 27 (patched)
> > 
> >
> > 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?
> 
> Andrei Sekretenko wrote:
> Right at this moment, this also helps minimize things that are kept in 
> the public header `` (which, for example, is 
> included into something like 1/3 of the volume of our tests, directly or 
> indirectly).
> 
> Benjamin Mahler wrote:
> Oh, just to minimize the content of ``? 
> We're only saving 1 small struct here, doesn't seem worth it? Or have you 
> seen it actually make a difference in compile times?
> 
> I would suspect we have better low hanging fruit for improving compile 
> times more generally?

This is not a question of a clean compile time but of a recompile time.

When any change in the filter creation interface - an internal interface which 
is absolutely of no concern to the code that mocks `TestAllocator` and mostly 
of no concern to all the other stuff that includes `allocator.hpp` - results in 
recompiling 1/4 of the project, this just does not look right.

Sure, we have a very similar issue with protobufs. 
However, protobufs are by design essentially append-only, which is not the case 
with the interfaces internal to Mesos (or, at least, should not be the case).

Overall, in the public headers we have around 140 non-protobuf class/struct 
definitions, most of which are really needed there (used by modules/schedulers).

--

On the other hand, the construction interface will probably end up converted 
into a factory-like entity in future.
Given that this is likely to happen before we add more options, I'm inclined to 
stop thinking about this particular case and just define `struct Options` in 
this header, but leave a TODO for pulling the construction interface away from 
the public header in case we find ourselves adding more options and this 
`struct Options` is still here.


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

Re: Review Request 72786: Implemented regex-based offer constraints.

[Benjamin Mahler]

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


Ship it!




Looks good, modulo the OfferConstraintsFilter / OfferConstraintsFilter::Options 
being split into separate headers.


src/master/allocator/mesos/offer_constraints_filter.cpp
Line 59 (original), 58 (patched)


Oh, didn't realize RE2 doesn't have a move constructor..

I guess we may want to make note of that somewhere, maybe in the top of 
this function?


- Benjamin Mahler


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

Re: Review Request 72786: Implemented regex-based offer constraints.

[Benjamin Mahler]

> On Aug. 26, 2020, 11:28 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/offer_constraints_filter.hpp
> > Lines 27 (patched)
> > 
> >
> > 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?
> 
> Andrei Sekretenko wrote:
> Right at this moment, this also helps minimize things that are kept in 
> the public header `` (which, for example, is 
> included into something like 1/3 of the volume of our tests, directly or 
> indirectly).

Oh, just to minimize the content of ``? We're 
only saving 1 small struct here, doesn't seem worth it? Or have you seen it 
actually make a difference in compile times?

I would suspect we have better low hanging fruit for improving compile times 
more generally?


- Benjamin


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

Re: Review Request 72786: Implemented regex-based offer constraints.

[Andrei Sekretenko]

> On Aug. 26, 2020, 11:28 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/offer_constraints_filter.hpp
> > Lines 27 (patched)
> > 
> >
> > 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 `` (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)
> > 
> >
> > 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)
> > 
> >
> > 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 
> d724fd0a9832feea26f6db1a498b6bdee830 
>   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
> 
>

Re: Review Request 72786: Implemented regex-based offer constraints.

[Andrei Sekretenko]

---
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 (updated)
-

  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 
d724fd0a9832feea26f6db1a498b6bdee830 
  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/8/

Changes: https://reviews.apache.org/r/72786/diff/7-8/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 72786: Implemented regex-based offer constraints.

[Benjamin Mahler]

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




src/master/allocator/mesos/offer_constraints_filter.hpp
Lines 27 (patched)


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?



src/master/allocator/mesos/offer_constraints_filter.cpp
Lines 53-55 (patched)


Doesn't look like shared_ptr makes sense in this patch?



src/master/allocator/mesos/offer_constraints_filter.cpp
Lines 63-64 (patched)


How about opening and closing quote on same line?

```
return Error(
"Failed to construct regex from pattern"
" '" + regex + "': " + re2->error());
```



src/master/allocator/mesos/offer_constraints_filter.cpp
Lines 68 (patched)


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



src/master/constants.hpp
Lines 185-188 (patched)


Helpful! Probably we should mention which RE2 library version for these 
numbers, since I've read from discussion on github that the numbers have been 
observed to change across releases.



src/master/constants.hpp
Lines 189 (patched)


Or Kilobytes(4)?



src/tests/master/offer_constraints_filter_tests.cpp
Lines 46 (patched)


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;
```


- Benjamin Mahler


On Aug. 26, 2020, 12:23 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72786/
> ---
> 
> (Updated Aug. 26, 2020, 12:23 p.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 
> d724fd0a9832feea26f6db1a498b6bdee830 
>   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/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 72786: Implemented regex-based offer constraints.

[Andrei Sekretenko]

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

(Updated Aug. 26, 2020, 12:23 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Replaced filter factory with `create()` options.


Bugs: MESOS-10173
https://issues.apache.org/jira/browse/MESOS-10173


Repository: mesos


Description
---

Implemented regex-based offer constraints.


Diffs (updated)
-

  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 
d724fd0a9832feea26f6db1a498b6bdee830 
  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/7/

Changes: https://reviews.apache.org/r/72786/diff/6-7/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 72786: Implemented regex-based offer constraints.

[Andrei Sekretenko]

> On Aug. 24, 2020, 9:30 p.m., Benjamin Mahler wrote:
> > Hm.. is it possible to have a simpler patch that avoids the factory and 
> > instead passes the flags through the creation path?
> 
> Andrei Sekretenko wrote:
> This is possible; however, I would argue that putting limits on RE2 is 
> rather deep implemenation detail which should not pollute the **public 
> allocator interface**.
> 
> Given this, and also the need for caching, I'm not sure that passing the 
> flags through creation path and removing them right after that makes a lot of 
> sense.

Realized two things: 
 - that I will end up refactoring the factory when adding caching if I leave 
the factory like this
 - that there is no need to have the definiotion of the filter options in the 
public allocator interface; they can be just forward-declared.

Removed the factory for now.


- Andrei


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


On Aug. 26, 2020, 12:23 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72786/
> ---
> 
> (Updated Aug. 26, 2020, 12:23 p.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 
> d724fd0a9832feea26f6db1a498b6bdee830 
>   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/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 72786: Implemented regex-based offer constraints.

[Andrei Sekretenko]

> On Aug. 24, 2020, 9:30 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/offer_constraints_filter.cpp
> > Lines 113-119 (patched)
> > 
> >
> > How did you figure this out? Just from your own testing?
> > 
> > What about with clang? Same result?
> > 
> > How much faster? Is it worth bothering?
> 
> Andrei Sekretenko wrote:
> Around 10% of the allocation loop time in a rather pecualr, although not 
> exotic, situation: a fast regexp that filters out most of the agents.
> Probably not worth bothering until we have a well-parametrized benchmark.
> 
> Maybe instead I should figure out if -O3 (which is the default in ther 
> Makefile, as opposed to cMake with RelWithDebInfo, which by defautl uses -O2) 
> can get us rid of this buffer zeroing: 
> https://github.com/google/re2/blob/ca11026a032ce2a3de4b3c389ee53d2bdc8794d6/re2/re2.cc#L907
> in the case of FullMatch.
> 
> If it does, it might be worth making the CMake build less consistent 
> internally but more consistent with the "standard" build flags of RE2.

Hmm.. now I don't see such a large effect with the draft benchmark I have at 
hand; either I've lost the version that was exhibiting that 10% difference or 
that was a measurement error.
Tried -O3, that doesn't help to optimize out that `memset` in `RE2::DoMatch()`. 
Dropped this wrapper.


> On Aug. 24, 2020, 9:30 p.m., Benjamin Mahler wrote:
> > src/master/constants.hpp
> > Lines 182 (patched)
> > 
> >
> > regular
> > 
> > Anything you can add about how these numbers were produced? Would be 
> > helpful.

Added an example regex similar to what we saw in some of the real-world 
Marathon apps and extracted the minimum max_mem under which it constructs 
successfully.


- Andrei


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


On Aug. 25, 2020, 6:43 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72786/
> ---
> 
> (Updated Aug. 25, 2020, 6:43 p.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 
> d724fd0a9832feea26f6db1a498b6bdee830 
>   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/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 72786: Implemented regex-based offer constraints.

[Andrei Sekretenko]

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

(Updated Aug. 25, 2020, 6:43 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

- switched the `max_mem` flag to `Bytes`, fixed the type of 
`DEFAULT_OFFER_CONSTRAINTS_RE2_MAX_PROGRAM_SIZE`
- dropped the local `fullMatch()` wrapper


Bugs: MESOS-10173
https://issues.apache.org/jira/browse/MESOS-10173


Repository: mesos


Description
---

Implemented regex-based offer constraints.


Diffs (updated)
-

  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 
d724fd0a9832feea26f6db1a498b6bdee830 
  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/6/

Changes: https://reviews.apache.org/r/72786/diff/5-6/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 72786: Implemented regex-based offer constraints.

[Andrei Sekretenko]

> On Aug. 24, 2020, 9:30 p.m., Benjamin Mahler wrote:
> > Hm.. is it possible to have a simpler patch that avoids the factory and 
> > instead passes the flags through the creation path?

This is possible; however, I would argue that putting limits on RE2 is rather 
deep implemenation detail which should not pollute the **public allocator 
interface**.

Given this, and also the need for caching, I'm not sure that passing the flags 
through creation path and removing them right after that makes a lot of sense.


> On Aug. 24, 2020, 9:30 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/offer_constraints_filter.cpp
> > Lines 113-119 (patched)
> > 
> >
> > How did you figure this out? Just from your own testing?
> > 
> > What about with clang? Same result?
> > 
> > How much faster? Is it worth bothering?

Around 10% of the allocation loop time in a rather pecualr, although not 
exotic, situation: a fast regexp that filters out most of the agents.
Probably not worth bothering until we have a well-parametrized benchmark.

Maybe instead I should figure out if -O3 (which is the default in ther 
Makefile, as opposed to cMake with RelWithDebInfo, which by defautl uses -O2) 
can get us rid of this buffer zeroing: 
https://github.com/google/re2/blob/ca11026a032ce2a3de4b3c389ee53d2bdc8794d6/re2/re2.cc#L907
in the case of FullMatch.

If it does, it might be worth making the CMake build less consistent internally 
but more consistent with the "standard" build flags of RE2.


> On Aug. 24, 2020, 9:30 p.m., Benjamin Mahler wrote:
> > src/master/flags.hpp
> > Lines 125-126 (patched)
> > 
> >
> > Just noticed that these don't line up with the above constant types, is 
> > that intentional?
> > 
> > Can we make max_mem of type `Bytes`? That would let a user specify 
> > "4kb", "4MB" etc

These are types are identical to those used in RE2. 
I'll try `Bytes`, though; if flags work with that, that would be great.


- Andrei


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


On Aug. 24, 2020, 7:46 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72786/
> ---
> 
> (Updated Aug. 24, 2020, 7:46 p.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 
> ef8a948260d92a9d54efb65f0e5c8aa3d0c67b91 
>   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 
> f88e2011bc510323344ffe58fd550b7683242950 
> 
> 
> Diff: https://reviews.apache.org/r/72786/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 72786: Implemented regex-based offer constraints.

[Benjamin Mahler]

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



Hm.. is it possible to have a simpler patch that avoids the factory and instead 
passes the flags through the creation path?


src/master/allocator/mesos/offer_constraints_filter.cpp
Lines 113-119 (patched)


How did you figure this out? Just from your own testing?

What about with clang? Same result?

How much faster? Is it worth bothering?



src/master/allocator/mesos/offer_constraints_filter.cpp
Lines 230 (patched)


Seems more readable to format this and the one below the same? (i.e. either 
both on two lines or both on one?)



src/master/constants.hpp
Lines 182 (patched)


regular

Anything you can add about how these numbers were produced? Would be 
helpful.



src/master/flags.hpp
Lines 125-126 (patched)


Just noticed that these don't line up with the above constant types, is 
that intentional?

Can we make max_mem of type `Bytes`? That would let a user specify "4kb", 
"4MB" etc


- Benjamin Mahler


On Aug. 24, 2020, 7:46 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72786/
> ---
> 
> (Updated Aug. 24, 2020, 7:46 p.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 
> ef8a948260d92a9d54efb65f0e5c8aa3d0c67b91 
>   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 
> f88e2011bc510323344ffe58fd550b7683242950 
> 
> 
> Diff: https://reviews.apache.org/r/72786/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 72786: Implemented regex-based offer constraints.

[Andrei Sekretenko]

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

(Updated Aug. 24, 2020, 7:46 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

- Separated regex caching from the initial implementation of the regex 
constraints.
 - Parametrized the RE2 limits.
 - Adjusted for constraints rename.


Bugs: MESOS-10173
https://issues.apache.org/jira/browse/MESOS-10173


Repository: mesos


Description
---

Implemented regex-based offer constraints.


Diffs (updated)
-

  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 
ef8a948260d92a9d54efb65f0e5c8aa3d0c67b91 
  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 
f88e2011bc510323344ffe58fd550b7683242950 


Diff: https://reviews.apache.org/r/72786/diff/5/

Changes: https://reviews.apache.org/r/72786/diff/4-5/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 72786: Implemented regex-based offer constraints.

[Andrei Sekretenko]

> On Aug. 20, 2020, 11:33 p.m., Benjamin Mahler wrote:
> > Could you split this into two logical changes?
> > 
> > 1. Add the regex support w/o any caching
> > 2. Add caching of constructed regexes as a memory / performance optimization
> > 
> > It seems like the caching in itself warrants some thought, and so it would 
> > be easier to review them in isolation.

Makes sense; will do that.


> On Aug. 20, 2020, 11:33 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/offer_constraints_filter.cpp
> > Lines 47-49 (patched)
> > 
> >
> > Some documentation on these and their values would be helpful.
> > 
> > Wow, max_mem is rather complicated, and it looks hard to be confident 
> > about our choice of value:
> > 
> > https://github.com/google/re2/blob/2020-08-01/re2/re2.h#L591-L617
> > 
> > Although that reads as though there is some caching / flexibility on 
> > the memory usage, I assume there is also a hard error if it can't construct 
> > the appropriately initial data structures?
> > 
> > Given this, it's a little scary to not have a tunable flag for it, 
> > should we run into surprising results in practice.
> > 
> > Similarly, for max program size of 100, it's really hard to see how 
> > this value is produced, and while 100 seems like a standard upper bound 
> > (according to envoy docs), it's a little scary to not have a tunable flag 
> > for this one too.
> > 
> > Thoughts?

Yes, there is a hard error if RE2 cannot be constructed without breaching the 
limit.

Not having max_mem / program size as configurable flags indeed seems scary, 
I'll change that.
 
My initial thinking was that having these configurable makes it more complex 
for the frameworks to validate the regexps on their side (so that the 
frameworks can avoid failing subscription/update).
With those limits being configurable, frameworks will need to somehow know the 
exact values of these limits (or Mesos will have to provide a validation API).

Probably you are right, and having those configurable right from the start is a 
lesser evil.


> On Aug. 20, 2020, 11:33 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/offer_constraints_filter.cpp
> > Lines 60-62 (patched)
> > 
> >
> > Thinking out loud here:
> > 
> > It's really hard for me to tell from here, but the weak cache will 
> > cover the typical path of a scheduler just updating to the same constraint 
> > info as before, right?
> > 
> > Caching wise, it seems most important to (1) avoid the duplication of 
> > regexes within a scheduler, and (2) secondarily avoid the duplication of 
> > regexes across time and across schedulers.
> > 
> > I can see how this covers the second case, since there will clearly be 
> > other references to regexes used by other schedulers. But the first case is 
> > a little less clear because it relies on the caller further up the stack to 
> > not throw away the old constraints object before making the new one, right? 
> > Is that ensured because the allocator will hold a copy, and the master 
> > constructs the new constraints before giving to the allocator to overwrite 
> > the old? This seems like a lot of non-local reasoning needed.
> > 
> > Just makes me wonder if there's a more direct way to cache (where the 
> > caching behavior is clear locally, with less assumption about the caller 
> > baked in). We could consider caching only across a single scheduler's 
> > updates if that's very simple to do and reason about.

Ideally, caching of RE2s should accomplish two goals:
1. Minimize the memory footprint of the used regexps.
2. Avoid re-creating the same regex while keeping the excess memory usage low.

Caching of weak references serves the first goal; it guarantees the minimum 
possible total memory usage of RE2s.
I'm not sure if it is reasonable to assume that different schedulers will use 
different regexps, especially the complex ones.
I can easily imagine a complex regexp selecting 97 agents out of 500 written 
down somewhere on the operator's internal Wiki and copy-pasted from one 
scheduler to another.

On the other hand, the second goal is always a kind of CPU/memory tradeoff.
I fully agree that the fact that weak reference caching also helps with this 
one looks like a coincidence.

Worse, some frameworks in some situations (for example, Marathon with a 
crashlooping app) will find themselves constantly switching a constraint (or, 
more likely, a whole constraint group) on/off. 

Weak caching does not help at all in this case; this is why I'm using an LRU, 
but probably it is worth looking for something better than a global LRU with an 
arbitrary fixed size of 1024.

I would expect that the constraints are extremely unlikely to "jump" from one 
framework to another, so this indeed can b

Re: Review Request 72786: Implemented regex-based offer constraints.

[Benjamin Mahler]

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



Could you split this into two logical changes?

1. Add the regex support w/o any caching
2. Add caching of constructed regexes as a memory / performance optimization

It seems like the caching in itself warrants some thought, and so it would be 
easier to review them in isolation.


src/master/allocator/mesos/offer_constraints_filter.cpp
Lines 47-49 (patched)


Some documentation on these and their values would be helpful.

Wow, max_mem is rather complicated, and it looks hard to be confident about 
our choice of value:

https://github.com/google/re2/blob/2020-08-01/re2/re2.h#L591-L617

Although that reads as though there is some caching / flexibility on the 
memory usage, I assume there is also a hard error if it can't construct the 
appropriately initial data structures?

Given this, it's a little scary to not have a tunable flag for it, should 
we run into surprising results in practice.

Similarly, for max program size of 100, it's really hard to see how this 
value is produced, and while 100 seems like a standard upper bound (according 
to envoy docs), it's a little scary to not have a tunable flag for this one too.

Thoughts?



src/master/allocator/mesos/offer_constraints_filter.cpp
Lines 60-62 (patched)


Thinking out loud here:

It's really hard for me to tell from here, but the weak cache will cover 
the typical path of a scheduler just updating to the same constraint info as 
before, right?

Caching wise, it seems most important to (1) avoid the duplication of 
regexes within a scheduler, and (2) secondarily avoid the duplication of 
regexes across time and across schedulers.

I can see how this covers the second case, since there will clearly be 
other references to regexes used by other schedulers. But the first case is a 
little less clear because it relies on the caller further up the stack to not 
throw away the old constraints object before making the new one, right? Is that 
ensured because the allocator will hold a copy, and the master constructs the 
new constraints before giving to the allocator to overwrite the old? This seems 
like a lot of non-local reasoning needed.

Just makes me wonder if there's a more direct way to cache (where the 
caching behavior is clear locally, with less assumption about the caller baked 
in). We could consider caching only across a single scheduler's updates if 
that's very simple to do and reason about.


- Benjamin Mahler


On Aug. 17, 2020, 8:23 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72786/
> ---
> 
> (Updated Aug. 17, 2020, 8:23 p.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 
> c700528e14bb42f6cea37f42dd7b72eb76a1a6b9 
>   src/master/allocator/mesos/offer_constraints_filter.cpp PRE-CREATION 
>   src/master/master.hpp 214307fcae47905672260758a1b96a034ed80257 
>   src/master/master.cpp 6a013e334b19bd108791d1c5fd0864c710aae8cb 
>   src/tests/master/offer_constraints_filter_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72786/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Review Request 72786: Implemented regex-based offer constraints.

[Andrei Sekretenko]

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

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 
c700528e14bb42f6cea37f42dd7b72eb76a1a6b9 
  src/master/allocator/mesos/offer_constraints_filter.cpp PRE-CREATION 
  src/master/master.hpp 214307fcae47905672260758a1b96a034ed80257 
  src/master/master.cpp 6a013e334b19bd108791d1c5fd0864c710aae8cb 
  src/tests/master/offer_constraints_filter_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/72786/diff/1/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 72786: Implemented regex-based offer constraints.

[Andrei Sekretenko]

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




src/master/allocator/mesos/offer_constraints_filter.cpp
Lines 117 (patched)


Published the PR and right after that realized that `RE2`s in the error 
state contain some kind of an error string. I should use that here.


- Andrei Sekretenko


On Aug. 17, 2020, 8:23 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72786/
> ---
> 
> (Updated Aug. 17, 2020, 8:23 p.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 
> c700528e14bb42f6cea37f42dd7b72eb76a1a6b9 
>   src/master/allocator/mesos/offer_constraints_filter.cpp PRE-CREATION 
>   src/master/master.hpp 214307fcae47905672260758a1b96a034ed80257 
>   src/master/master.cpp 6a013e334b19bd108791d1c5fd0864c710aae8cb 
>   src/tests/master/offer_constraints_filter_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72786/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>