You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benno Evers <be...@mesosphere.com> on 2017/12/04 16:29:53 UTC

Re: Review Request 64010: Added `SlaveInfo` parameter to Allocator::updateSlave().

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

(Updated Dec. 4, 2017, 4:29 p.m.)


Review request for mesos and Vinod Kone.


Summary (updated)
-----------------

Added `SlaveInfo` parameter to Allocator::updateSlave().


Repository: mesos


Description (updated)
-------

This is done mainly in preparation for the upcoming agent
reconfiguration patches, where it will become possible that
`updateSlave()` is passed a different SlaveInfo than `addSlave()`,
and schedulers who rely on some fields in it for their scheduling
decisions need to be able to check and compare them.

Additionally, the HierarchicalDRFAllocator was changed to store
the full SlaveInfo for every slave instead of storing domain and
hostname separately.


Diffs (updated)
-----

  include/mesos/allocator/allocator.hpp acb9e4f6a843e64c915c43c218d8a533ca63333b 
  src/master/allocator/mesos/allocator.hpp 48254b6e0974bdc16ffda04d3d271538048d3206 
  src/master/allocator/mesos/hierarchical.hpp 3c87dc797cf70f3aa48b1ed9f86d673d4ea2fe76 
  src/master/allocator/mesos/hierarchical.cpp 715650ee9cb15aed1d1e58badf70fc09e26d13c1 
  src/master/master.cpp 883793a226849434eb833c3027d121635a86fdc4 
  src/tests/allocator.hpp fc5d9efc8dab3bd971fa8938e3f82e8291c4ab9d 
  src/tests/hierarchical_allocator_tests.cpp 0309074bab180be122c9b0074981e6f69c97feee 


Diff: https://reviews.apache.org/r/64010/diff/6/

Changes: https://reviews.apache.org/r/64010/diff/5-6/


Testing
-------


Thanks,

Benno Evers


Re: Review Request 64010: Added `SlaveInfo` parameter to Allocator::updateSlave().

Posted by Vinod Kone <vi...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64010/#review192767
-----------------------------------------------------------


Fix it, then Ship it!





include/mesos/allocator/allocator.hpp
Line 210 (original), 211 (patched)
<https://reviews.apache.org/r/64010/#comment271020>

    Can you add a TODO to make `total` and `capabilities` non-optional too?



src/master/allocator/mesos/hierarchical.hpp
Lines 386 (patched)
<https://reviews.apache.org/r/64010/#comment271021>

    s/domain/host/


- Vinod Kone


On Dec. 4, 2017, 4:29 p.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64010/
> -----------------------------------------------------------
> 
> (Updated Dec. 4, 2017, 4:29 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This is done mainly in preparation for the upcoming agent
> reconfiguration patches, where it will become possible that
> `updateSlave()` is passed a different SlaveInfo than `addSlave()`,
> and schedulers who rely on some fields in it for their scheduling
> decisions need to be able to check and compare them.
> 
> Additionally, the HierarchicalDRFAllocator was changed to store
> the full SlaveInfo for every slave instead of storing domain and
> hostname separately.
> 
> 
> Diffs
> -----
> 
>   include/mesos/allocator/allocator.hpp acb9e4f6a843e64c915c43c218d8a533ca63333b 
>   src/master/allocator/mesos/allocator.hpp 48254b6e0974bdc16ffda04d3d271538048d3206 
>   src/master/allocator/mesos/hierarchical.hpp 3c87dc797cf70f3aa48b1ed9f86d673d4ea2fe76 
>   src/master/allocator/mesos/hierarchical.cpp 715650ee9cb15aed1d1e58badf70fc09e26d13c1 
>   src/master/master.cpp 883793a226849434eb833c3027d121635a86fdc4 
>   src/tests/allocator.hpp fc5d9efc8dab3bd971fa8938e3f82e8291c4ab9d 
>   src/tests/hierarchical_allocator_tests.cpp 0309074bab180be122c9b0074981e6f69c97feee 
> 
> 
> Diff: https://reviews.apache.org/r/64010/diff/6/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> https://jenkins.mesosphere.com/service/jenkins/view/Mesos/job/mesos/job/Mesos_CI-build/2299/
> 
> 
> Thanks,
> 
> Benno Evers
> 
>


Re: Review Request 64010: Added `SlaveInfo` parameter to Allocator::updateSlave().

Posted by Vinod Kone <vi...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64010/#review192903
-----------------------------------------------------------


Ship it!




Ship It!

- Vinod Kone


On Dec. 5, 2017, 5:39 p.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64010/
> -----------------------------------------------------------
> 
> (Updated Dec. 5, 2017, 5:39 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This is done mainly in preparation for the upcoming agent
> reconfiguration patches, where it will become possible that
> `updateSlave()` is passed a different SlaveInfo than `addSlave()`,
> and schedulers who rely on some fields in it for their scheduling
> decisions need to be able to check and compare them.
> 
> Additionally, the HierarchicalDRFAllocator was changed to store
> the full SlaveInfo for every slave instead of storing domain and
> hostname separately.
> 
> 
> Diffs
> -----
> 
>   include/mesos/allocator/allocator.hpp acb9e4f6a843e64c915c43c218d8a533ca63333b 
>   src/master/allocator/mesos/allocator.hpp 48254b6e0974bdc16ffda04d3d271538048d3206 
>   src/master/allocator/mesos/hierarchical.hpp 41ffe17def133d7e735afa26a6fedd154e51f4b4 
>   src/master/allocator/mesos/hierarchical.cpp 4428541e63561f7c3bc3bc61f4b47e35216f6442 
>   src/master/master.cpp 31991b5f7f7f4e7dc73d71a3160348bfc0a627c3 
>   src/tests/allocator.hpp fc5d9efc8dab3bd971fa8938e3f82e8291c4ab9d 
>   src/tests/hierarchical_allocator_tests.cpp 154049c47f2dae2df8d1bb4ddb5c48c478bb3d0e 
> 
> 
> Diff: https://reviews.apache.org/r/64010/diff/8/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> https://jenkins.mesosphere.com/service/jenkins/view/Mesos/job/mesos/job/Mesos_CI-build/2299/
> 
> 
> Thanks,
> 
> Benno Evers
> 
>


Re: Review Request 64010: Added `SlaveInfo` parameter to Allocator::updateSlave().

Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64010/
-----------------------------------------------------------

(Updated Dec. 5, 2017, 9:26 p.m.)


Review request for mesos and Vinod Kone.


Changes
-------

Rebased onto master.


Repository: mesos


Description
-------

This is done mainly in preparation for the upcoming agent
reconfiguration patches, where it will become possible that
`updateSlave()` is passed a different SlaveInfo than `addSlave()`,
and schedulers who rely on some fields in it for their scheduling
decisions need to be able to check and compare them.

Additionally, the HierarchicalDRFAllocator was changed to store
the full SlaveInfo for every slave instead of storing domain and
hostname separately.


Diffs (updated)
-----

  include/mesos/allocator/allocator.hpp acb9e4f6a843e64c915c43c218d8a533ca63333b 
  src/master/allocator/mesos/allocator.hpp 48254b6e0974bdc16ffda04d3d271538048d3206 
  src/master/allocator/mesos/hierarchical.hpp 41ffe17def133d7e735afa26a6fedd154e51f4b4 
  src/master/allocator/mesos/hierarchical.cpp 4bc9fb6f787224114f1285937cf979ace46d8ea3 
  src/master/master.cpp e8257e786b411a0f569559526918a6c49c63875a 
  src/tests/allocator.hpp fc5d9efc8dab3bd971fa8938e3f82e8291c4ab9d 
  src/tests/hierarchical_allocator_tests.cpp 154049c47f2dae2df8d1bb4ddb5c48c478bb3d0e 


Diff: https://reviews.apache.org/r/64010/diff/9/

Changes: https://reviews.apache.org/r/64010/diff/8-9/


Testing
-------

`make check`

https://jenkins.mesosphere.com/service/jenkins/view/Mesos/job/mesos/job/Mesos_CI-build/2299/


Thanks,

Benno Evers


Re: Review Request 64010: Added `SlaveInfo` parameter to Allocator::updateSlave().

Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64010/
-----------------------------------------------------------

(Updated Dec. 5, 2017, 5:39 p.m.)


Review request for mesos and Vinod Kone.


Changes
-------

Rebased onto latest master.


Repository: mesos


Description
-------

This is done mainly in preparation for the upcoming agent
reconfiguration patches, where it will become possible that
`updateSlave()` is passed a different SlaveInfo than `addSlave()`,
and schedulers who rely on some fields in it for their scheduling
decisions need to be able to check and compare them.

Additionally, the HierarchicalDRFAllocator was changed to store
the full SlaveInfo for every slave instead of storing domain and
hostname separately.


Diffs (updated)
-----

  include/mesos/allocator/allocator.hpp acb9e4f6a843e64c915c43c218d8a533ca63333b 
  src/master/allocator/mesos/allocator.hpp 48254b6e0974bdc16ffda04d3d271538048d3206 
  src/master/allocator/mesos/hierarchical.hpp 41ffe17def133d7e735afa26a6fedd154e51f4b4 
  src/master/allocator/mesos/hierarchical.cpp 4428541e63561f7c3bc3bc61f4b47e35216f6442 
  src/master/master.cpp 31991b5f7f7f4e7dc73d71a3160348bfc0a627c3 
  src/tests/allocator.hpp fc5d9efc8dab3bd971fa8938e3f82e8291c4ab9d 
  src/tests/hierarchical_allocator_tests.cpp 154049c47f2dae2df8d1bb4ddb5c48c478bb3d0e 


Diff: https://reviews.apache.org/r/64010/diff/8/

Changes: https://reviews.apache.org/r/64010/diff/7-8/


Testing
-------

`make check`

https://jenkins.mesosphere.com/service/jenkins/view/Mesos/job/mesos/job/Mesos_CI-build/2299/


Thanks,

Benno Evers


Re: Review Request 64010: Added `SlaveInfo` parameter to Allocator::updateSlave().

Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64010/
-----------------------------------------------------------

(Updated Dec. 4, 2017, 11:30 p.m.)


Review request for mesos and Vinod Kone.


Repository: mesos


Description
-------

This is done mainly in preparation for the upcoming agent
reconfiguration patches, where it will become possible that
`updateSlave()` is passed a different SlaveInfo than `addSlave()`,
and schedulers who rely on some fields in it for their scheduling
decisions need to be able to check and compare them.

Additionally, the HierarchicalDRFAllocator was changed to store
the full SlaveInfo for every slave instead of storing domain and
hostname separately.


Diffs (updated)
-----

  include/mesos/allocator/allocator.hpp acb9e4f6a843e64c915c43c218d8a533ca63333b 
  src/master/allocator/mesos/allocator.hpp 48254b6e0974bdc16ffda04d3d271538048d3206 
  src/master/allocator/mesos/hierarchical.hpp 3c87dc797cf70f3aa48b1ed9f86d673d4ea2fe76 
  src/master/allocator/mesos/hierarchical.cpp 715650ee9cb15aed1d1e58badf70fc09e26d13c1 
  src/master/master.cpp 883793a226849434eb833c3027d121635a86fdc4 
  src/tests/allocator.hpp fc5d9efc8dab3bd971fa8938e3f82e8291c4ab9d 
  src/tests/hierarchical_allocator_tests.cpp 0309074bab180be122c9b0074981e6f69c97feee 


Diff: https://reviews.apache.org/r/64010/diff/7/

Changes: https://reviews.apache.org/r/64010/diff/6-7/


Testing
-------

`make check`

https://jenkins.mesosphere.com/service/jenkins/view/Mesos/job/mesos/job/Mesos_CI-build/2299/


Thanks,

Benno Evers