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