mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 72786: Implemented regex-based offer constraints.
Date Thu, 20 Aug 2020 23:33:17 GMT

-----------------------------------------------------------
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)
<https://reviews.apache.org/r/72786/#comment310732>

    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)
<https://reviews.apache.org/r/72786/#comment310734>

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


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