mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joris Van Remoortere" <joris.van.remoort...@gmail.com>
Subject Re: Review Request 33376: MESOS-2633 Moved struct Framework methods to their own implementation class.
Date Wed, 29 Apr 2015 20:24:42 GMT

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



src/master/framework.cpp
<https://reviews.apache.org/r/33376/#comment132580>

    I think having this comment here (together with the implementation) is good. Can we remove
it from the header so we don't have it duplicated?



src/master/framework.cpp
<https://reviews.apache.org/r/33376/#comment132581>

    This is not your fault:
    there was a patch that got committed that changes `memory::shared_ptr` to `std::shared_ptr`.
Can you make sure that when this gets rebased that we get rid of any reference to `memory:shared_ptr`?
Thanks!



src/master/framework.cpp
<https://reviews.apache.org/r/33376/#comment132583>

    add a new line between the closing `}` and `offers.erase(offer)`



src/master/framework.cpp
<https://reviews.apache.org/r/33376/#comment132587>

    The style is either:
    Follow the alignment if you stay on the 1st line:
    ```
    bool Framework::hasExecutor(const SlaveID& slaveId,
                                const ExecutorID& executorId)
    ```
    or
    Indent by 4 if you start the parameter list on a new line:
    ```
    bool Framework::hasExecutor(
        const SlaveID& slaveId,
        const ExecutorID& executorId)
    ```
    
    Here and the others below please!



src/master/framework.cpp
<https://reviews.apache.org/r/33376/#comment132588>

    I think you changed the indentation here.
    This is the original:
    ```
    return executors.contains(slaveId) &&
      executors[slaveId].contains(executorId);
    ```
    Check out the Indentation -> Continuation section in the style guide:
    http://mesos.apache.org/documentation/latest/mesos-c++-style-guide/



src/master/framework.cpp
<https://reviews.apache.org/r/33376/#comment132590>

    please leave the original new line between the closing `}` and the `executors[slaveId].erase(executorId);`
line.



src/master/framework.cpp
<https://reviews.apache.org/r/33376/#comment132592>

    Can you find other places in the code where the TODOs are aligned like this?
    It's definitely neat, but it seems to be different from other places.



src/master/framework.cpp
<https://reviews.apache.org/r/33376/#comment132593>

    1) Let's sync with BenH if we want to factor out logging like this. There are arguments
on both sides, so let's follow up with a discussion, and maybe add it to the style guide!
    2) *If* we *do* decide to factor out the logging like this, let's change the name to be
more meaninful, or even move this function into `updateFrameworkInfo` as a lambda. This message
is clearly specific to that function, and is not usable by other functions in `Framework`.
    3) Since we're in the implementation file, we can do `using std::string` and remove the
namespacing of `std::string` here.


- Joris Van Remoortere


On April 22, 2015, 8:35 p.m., Marco Massenzio wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33376/
> -----------------------------------------------------------
> 
> (Updated April 22, 2015, 8:35 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-2633
>     https://issues.apache.org/jira/browse/MESOS-2633
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Created new file framework.cpp containing all the methods' implementations for the `Framework`
class (declared in master/master.hpp)
> 
> Declared `operator ==` for Task in type_utils.hpp 
> (it was implemented before, but not declared in the header file);
> 
> Refactored all the LOG(WARNING) to a single utility method.
> 
> 
> Diffs
> -----
> 
>   include/mesos/type_utils.hpp cdf5864389a72002b538c263d70bcade2bdffa45 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/master/framework.cpp PRE-CREATION 
>   src/master/master.hpp 550d2c50cd01aa5830a7e7e03ab4a0240c221b15 
> 
> Diff: https://reviews.apache.org/r/33376/diff/
> 
> 
> Testing
> -------
> 
> All tests (make check) pass.
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>


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