mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joseph Wu" <jos...@mesosphere.io>
Subject Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.
Date Wed, 26 Aug 2015 21:20:03 GMT


> On Aug. 26, 2015, 12:03 p.m., Joris Van Remoortere wrote:
> > src/master/http.cpp, line 1448
> > <https://reviews.apache.org/r/37325/diff/12/?file=1051203#file1051203line1448>
> >
> >     I don't know if we require a new line here. I wouldn't mind one. thoughts?

It'll run over 80 characters without the newline.


> On Aug. 26, 2015, 12:03 p.m., Joris Van Remoortere wrote:
> > src/master/maintenance.hpp, line 37
> > <https://reviews.apache.org/r/37325/diff/12/?file=1051204#file1051204line37>
> >
> >     Do you want to use Doxygen documentation style since this is a new File? :-D

Completely forgot about that XD.


> On Aug. 26, 2015, 12:03 p.m., Joris Van Remoortere wrote:
> > src/master/maintenance.cpp, lines 93-96
> > <https://reviews.apache.org/r/37325/diff/12/?file=1051205#file1051205line93>
> >
> >     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?

I believe this was a leftover.  I'll remove it.


> On Aug. 26, 2015, 12:03 p.m., Joris Van Remoortere wrote:
> > src/master/maintenance.cpp, line 124
> > <https://reviews.apache.org/r/37325/diff/12/?file=1051205#file1051205line124>
> >
> >     Change numbers to something more meaningful?

This was literally when we were check the double for being not "Not-a-Number" :)
I'll update it.


> On Aug. 26, 2015, 12:03 p.m., Joris Van Remoortere wrote:
> > src/master/maintenance.cpp, line 136
> > <https://reviews.apache.org/r/37325/diff/12/?file=1051205#file1051205line136>
> >
> >     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?

Added a bunch of `lowercaseHostname` helper functions.


- Joseph


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


On Aug. 26, 2015, 2:16 p.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37325/
> -----------------------------------------------------------
> 
> (Updated Aug. 26, 2015, 2:16 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
> 
>


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