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

Not a full review, but stopping after the validation routines. Sharing early.

src/master/http.cpp (line 1377)

    We don't use `.` in our error messages.
    Do you want to include what value was passed for `method` in the error 

src/master/http.cpp (lines 1383 - 1387)

    Can we clean this up with something like:
    master->maintenanceSchedules.empty() ? mesos::maintenance::Schedule() : 
    Either as `schedule = ` or right in the return statement?

src/master/http.cpp (line 1401)

    new line between the assignment and the if. We do this if the assignment 
didn't fit in one line.

src/master/http.cpp (line 1410)

    new line.

src/master/http.cpp (lines 1421 - 1423)

    Do you want to use a c++11 lambda here instead?
    Then you can get rid of `continuation` above.

src/master/http.cpp (lines 1435 - 1463)

    It's not immediately obvious why you wouldn't just clear the 
maintenanceStatuses data structure and rebuild it, as opposed to keeping track 
of changes and updating, then removing the right things.
    I understand you're doing this because in the future this data structure 
will be holding information for more than 1 schedule.
    Can you add a comment here as to why we're going through these steps?

src/master/http.cpp (line 1443)


src/master/http.cpp (line 1448)

    I don't know if we require a new line here. I wouldn't mind one. thoughts?

src/master/maintenance.hpp (lines 26 - 27)

    is this the right order?

src/master/maintenance.hpp (line 37)

    Do you want to use Doxygen documentation style since this is a new File? :-D

src/master/maintenance.hpp (line 38)

    backticks (`) around the modes?

src/master/maintenance.hpp (line 39)

    s/non-Deactivated machines/machines that are not `Deactivated`

src/master/maintenance.hpp (line 60)

    I believe we like to indent lists like this

src/master/maintenance.hpp (line 61)

    Can we choose a different word than numbers? Data? TimeSpecs?

src/master/maintenance.cpp (lines 55 - 69)

    This code is an example of where we could benefit from a helper function if 
we had a C++ wrapper for these protos :-)

src/master/maintenance.cpp (line 73)


src/master/maintenance.cpp (line 84)


src/master/maintenance.cpp (lines 93 - 96)

    Did you explicitly want to alias here just to be used in the next line? Is 
this left over from when you were using `machineInfo` elsehwere?

src/master/maintenance.cpp (lines 116 - 117)

    Can you do a `foreach` here instead and iterate over `mutable_windows()` 
instead? I think that's the only reason you need `i` right?
    Larger question: why do we need mutable versions if we're just doing 

src/master/maintenance.cpp (line 121)

    Is it missing, or just empty?

src/master/maintenance.cpp (line 124)

    Change numbers to something more meaningful?

src/master/maintenance.cpp (line 126)

    indent 2 spaces for expression continuation.
    new line before if statement.

src/master/maintenance.cpp (line 131)

    Collect machines from the updated schedule into a set.

src/master/maintenance.cpp (line 132)

    Same thing here. Can you use a `foreach` on the mutable version?

src/master/maintenance.cpp (line 136)

    Is this the right place to perform this mutation?
    I would expect a validation routine to be side-effect free.
    I believe this is also the root of why you need mutable versions everywhere?
    Can we find a better place to do this, and just verify that the hostnames 
passed in are indeed lowercase?

src/master/maintenance.cpp (line 139)

    new line after multi-line expression.

src/master/maintenance.cpp (line 143)

    `Check that the machine is unique.` ?

src/master/maintenance.cpp (line 145)

    can you add a single-quote around the Machine name:
    `'" + machineInfo.DebugString() + "' is`

src/master/maintenance.cpp (line 146)

    2 space indent for expression continuation.

src/master/maintenance.cpp (line 153)

    How about:
    `Ensure no `Deactivated` machine is removed from the schedule.` ?

src/master/maintenance.cpp (line 154)


src/master/maintenance.cpp (line 157)

    can you add a single-quote around the Machine name:
    `'" + key.DebugString() + "' is`

src/master/maintenance.cpp (line 158)

    2 space indent for expression continuation.

src/master/maintenance.cpp (lines 170 - 172)

    2 space indent for expression continuation.

src/master/maintenance.cpp (line 176)

    Maybe add the value of the data to the message?
    Can we find something better than `numbers`?

src/master/maintenance.cpp (line 185)

    Comments end with a `.`
    Let's try and do this mutation elsewhere and just verify that the name is 
indeed lowercase here.
    That way we can get rid of the mutable aspects throughout this entire 
validation section.

- Joris Van Remoortere

On Aug. 25, 2015, 5:03 p.m., Joseph Wu wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37325/
> -----------------------------------------------------------
> (Updated Aug. 25, 2015, 5:03 p.m.)
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
> Joris Van Remoortere, and Vinod Kone.
> Bugs: MESOS-2067 and MESOS-3069
>     https://issues.apache.org/jira/browse/MESOS-2067
>     https://issues.apache.org/jira/browse/MESOS-3069
> Repository: mesos
> Description
> -------
> Endpoint: /maintenance.schedule
> Registry operation = maintenance::UpdateSchedule
>   Replaces the schedule with the given one.  Also sets all scheduled machines 
> into Draining mode.
> Other changes:
>   Added a note about the "strict" flag.
> Diffs
> -----
>   src/Makefile.am 9fd71d1ddf442712977596e7a13969ff5c1d68db 
>   src/master/http.cpp 37d76ee72f6a037f551bf2609e9393e16b496e44 
>   src/master/maintenance.hpp PRE-CREATION 
>   src/master/maintenance.cpp PRE-CREATION 
>   src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc 
>   src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf 
>   src/master/registrar.hpp c6a0655c212646618d93c9c85918af482a9ffd50 
>   src/tests/maintenance.hpp PRE-CREATION 
>   src/tests/master_maintenance_tests.cpp PRE-CREATION 
>   src/tests/registrar_tests.cpp 032e644ee19751b4ce5767d46f474d34ec4b9166 
> Diff: https://reviews.apache.org/r/37325/diff/
> Testing
> -------
> `make check`
> New Tests:
>   RegistrarTest.UpdateMaintenanceSchedule
>     Schedules 3 machines, 1 at a time.  Rearranges schedules.
>     Checks that machines are put into Draining mode.  Removes machines.
>   MasterMaintenanceTest.UpdateSchedule
>     Hits the new endpoint with some valid and invalid schedules.
>     Only tests a subset of invalid schedules (requires other endpoints to 
> fully test).
> Thanks,
> Joseph Wu

Reply via email to