This is an automatically generated e-mail. To reply, visit:

Hey Mandeep, Thanks for taking on this important work!
Some high level questions:
- Do we need to do reference counting manually or can we use a construct like 
`Shared` in Libprocess that wraps the `std::shared_ptr`.
- Do we want to surface the singleton pattern in this way? What are the impacts 
of this on the user (i.e. your logic calling the `acquire` / `release` code 
currently? Does the user care about holding a reference to the singleton?

Regardless, I added some style comments as they are valuable for you as a 

src/sched/sched.cpp (line 116)

    Can we provide some documentation as to the purpose of this class?
    You can reference the JIRA number within the comment.

src/sched/sched.cpp (line 120)

    We tend to return by pointer rather than reference, assuming we want to 
follow this form of singleton pattern.

src/sched/sched.cpp (line 121)

    no need for the space within `{ }`

src/sched/sched.cpp (line 123)

    `{` on a new line as per style guide. 
    You can also trim the `std::` from the `std::string` as we have a `using 
std::string` declaration above.
    Here and below.

src/sched/sched.cpp (lines 125 - 126)

    Can we add a comment here about the condition we're testing for, and then 
looking to achieve?

src/sched/sched.cpp (line 133)

    `{` on new line.

src/sched/sched.cpp (lines 135 - 144)

    Would this logic be easier to follow if we re-arranged it as:
    if (!masterDetectors.contains(url)) {
      return NULL;
    rest of logic...
    This is a pretty common pattern in our code base: early return conditions 
pulled to the top.

src/sched/sched.cpp (lines 136 - 137)

    Just some context: we try to avoid using aliases like this. Please see the 
comments in the style guide.
    In this particular case I think it is a valid use case, I just wanted to 
use this as a sharing opportunity :-)
    Can we rename `thisPair` to something more meaningful? Part of our argument 
for aliases is improved documentation through code.

src/sched/sched.cpp (line 140)

    You have an extra whitespace after refCount.

src/sched/sched.cpp (line 141)

    If the previous statement did not fit on a single line, we leave a blank 
line between it and the next statement.

src/sched/sched.cpp (line 148)

    `{` on new line.

src/sched/sched.cpp (lines 150 - 165)

    Similar comments as above.
    Let's take a look at this after you refactor the first section.

src/sched/sched.cpp (line 169)

    `{` on new line.

src/sched/sched.cpp (line 176)

    Comments end with a `.`

src/sched/sched.cpp (lines 182 - 184)

    In general, do we need to be doing reference counting manually?
    How come we can't use something like `Shared` in libprocess? It is a 
wrapper around `std::shared_ptr`.

src/sched/sched.cpp (lines 187 - 191)

    Do we need to provide an accessor to a singleton here?
    It makes the code below more terse.
    What do you think about having an internal singleton and making the 
functions themselves static, or having static wrappers?
    e.g. (using the acquire / release pattern)
    // Static functions.
    // Or no alias.
    or with `Shared`
    Shared<MasterDetector> detector = DetectorPool::acquire(...)

src/sched/sched.cpp (line 1806)

    indentation is off.

- Joris Van Remoortere

On Oct. 8, 2015, 5:05 p.m., Mandeep Chadha wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39060/
> -----------------------------------------------------------
> (Updated Oct. 8, 2015, 5:05 p.m.)
> Review request for mesos and Joris Van Remoortere.
> Bugs: MESOS-3595
>     https://issues.apache.org/jira/browse/MESOS-3595
> Repository: mesos
> Description
> -------
> If the number of framework created exceeds the lib process
> threads then during master failover the zookeeper updates can
> cause deadlock.
> Diffs
> -----
>   src/sched/sched.cpp 571e00d303009a940f17c8ed4582749a718e846d 
> Diff: https://reviews.apache.org/r/39060/diff/
> Testing
> -------
> make check successful 
> Created 100 framework instances on a 24 CPU machine. Master failover detected 
> by the framework process and continue to work as expected.
> Thanks,
> Mandeep Chadha

Reply via email to