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 70378: Wrapped access to hashmaps in `frameworks.principals` and `authorized`.
Date Wed, 03 Apr 2019 16:01:44 GMT

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

(Updated April 3, 2019, 4:01 p.m.)


Review request for mesos and Gastón Kleiman.


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


Repository: mesos


Description
-------

!! This code is completely disposable and possibly not properly located; the next patch actually
does not depend on this one.

------

After having a look at the Master code, I realized that the hashmaps `frameworks.principlas`
and `authorized` should satisfy the following property: the value corresponding to a given
key never changes.

Currently, this invariant is enforced MANUALLY with hacks of three kinds. 
The first hack is constructs like this:

CHECK(!h.contains(key));
h[key] = newValue;

The second hack is much worse: it is avoiding calling non-const `hashmap::operator[]()` for
nonexisting keys.

And the third one is even worse: it is avoiding this situation:
h.erase(key);
h[key] = newValue;

To make sure that at least the first two hacks has been used properly, I added this wrapper
around hashmap.
Actually, this is a dirty hack itself, and not a proper decomposition of the God Object antipattern
that, IMO, has somehow started to grow inside of the Master.

Unfortunately, there is no such simple way to validate that the third kind of hacks is used
properly. (In fact, it is not - and this is the direct cause of MESOS-2842.)


Diffs
-----

  src/master/master.hpp 94891af9deeaddb3333fc9d6eabb243aed97f7b7 
  src/master/master.cpp cf5caa0893ba1387a1f3a9d129ecd7d974f776bd 


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


Testing (updated)
-------

make check


Thanks,

Andrei Sekretenko


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