subject:"Re\: Review Request 25525\: MESOS\-1739\: Allow slave reconfiguration on restart"

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-10-24 Thread Vinod Kone


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


As discussed on IRC:

--> please split this into multiple reviews (attributes methods and tests, 
compatible method, allocator changes, rest)

--> fold slaveinfo_utils into protobuf_utils

--> rethink reregisterSlave()  (i'll help with this). one optimization i can 
think of which might help make reregisterSlave() simpler is to update the 
registrar to not do a log write if there is no mutation (e.g., readmit slave). 
It's fine if it seems too complicated and you want to punt on it too.

- Vinod Kone


On Oct. 22, 2014, 12:32 a.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25525/
> ---
> 
> (Updated Oct. 22, 2014, 12:32 a.m.)
> 
> 
> Review request for mesos, Adam B and Vinod Kone.
> 
> 
> Bugs: MESOS-1739
> https://issues.apache.org/jira/browse/MESOS-1739
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Allows attributes and resources to be set to a superset of what they were 
> previously on a slave restart.
> 
> Incorporates all comments from: 
> https://issues.apache.org/jira/browse/MESOS-1739
> and the former review request:
> https://reviews.apache.org/r/25111/
> 
> 
> Diffs
> -
> 
>   src/Makefile.am c44a9ad47d6e1262949b9049f4ae25b049440d99 
>   src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 
>   src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
>   src/common/slaveinfo_utils.hpp PRE-CREATION 
>   src/common/slaveinfo_utils.cpp PRE-CREATION 
>   src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223 
>   src/master/hierarchical_allocator_process.hpp 
> 31dfb2cc86e2e74da43f05fbada10976ce65f3e4 
>   src/master/master.hpp b1a2cd0f51f89d6dabbccaa67e0411fc55a4272f 
>   src/master/master.cpp e70cdbf78fd79738d38ecf314f9d8ee207558db2 
>   src/slave/slave.cpp 55e5264a61ce43970e177cf0eb27dc8a2657e261 
>   src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b 
>   src/tests/mesos.hpp e36e13843a36360d69a01f62356370bea588a337 
>   src/tests/slave_tests.cpp a1bd00cffe9de178b0b188c3556e479cd1f6d566 
> 
> Diff: https://reviews.apache.org/r/25525/diff/
> 
> 
> Testing
> ---
> 
> make check on localhost
> 
> 
> Thanks,
> 
> Cody Maloney
> 
>

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-10-21 Thread Mesos ReviewBot


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


Bad patch!

Reviews applied: [25525]

Failed command: ./support/apply-review.sh -n -r 25525

Error:
 --2014-10-22 00:59:50--  https://reviews.apache.org/r/25525/diff/raw/
Resolving reviews.apache.org (reviews.apache.org)... 140.211.11.74
Connecting to reviews.apache.org (reviews.apache.org)|140.211.11.74|:443... 
connected.
HTTP request sent, awaiting response... 200 OK
Length: 23628 (23K) [text/x-patch]
Saving to: '25525.patch'

 0K .. .. ... 100%  708K=0.03s

2014-10-22 00:59:50 (708 KB/s) - '25525.patch' saved [23628/23628]

Successfully applied: MESOS-1739: Allow slave reconfiguration on restart

Allows attributes and resources to be set to a superset of what they were 
previously on a slave restart.

Incorporates all comments from: 
https://issues.apache.org/jira/browse/MESOS-1739
and the former review request:
https://reviews.apache.org/r/25111/


Review: https://reviews.apache.org/r/25525
Checking 12 files using filter 
--filter=-,+build/class,+build/deprecated,+build/endif_comment,+readability/todo,+readability/namespace,+runtime/vlog,+whitespace/blank_line,+whitespace/comma,+whitespace/end_of_line,+whitespace/ending_newline,+whitespace/forcolon,+whitespace/indent,+whitespace/line_length,+whitespace/operators,+whitespace/semicolon,+whitespace/tab,+whitespace/todo
src/tests/slave_tests.cpp:1337:  Could not find a newline character at the end 
of the file.  [whitespace/ending_newline] [5]
Total errors found: 1
Failed to commit patch

- Mesos ReviewBot


On Oct. 22, 2014, 12:32 a.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25525/
> ---
> 
> (Updated Oct. 22, 2014, 12:32 a.m.)
> 
> 
> Review request for mesos, Adam B and Vinod Kone.
> 
> 
> Bugs: MESOS-1739
> https://issues.apache.org/jira/browse/MESOS-1739
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Allows attributes and resources to be set to a superset of what they were 
> previously on a slave restart.
> 
> Incorporates all comments from: 
> https://issues.apache.org/jira/browse/MESOS-1739
> and the former review request:
> https://reviews.apache.org/r/25111/
> 
> 
> Diffs
> -
> 
>   src/Makefile.am c44a9ad47d6e1262949b9049f4ae25b049440d99 
>   src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 
>   src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
>   src/common/slaveinfo_utils.hpp PRE-CREATION 
>   src/common/slaveinfo_utils.cpp PRE-CREATION 
>   src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223 
>   src/master/hierarchical_allocator_process.hpp 
> 31dfb2cc86e2e74da43f05fbada10976ce65f3e4 
>   src/master/master.hpp b1a2cd0f51f89d6dabbccaa67e0411fc55a4272f 
>   src/master/master.cpp e70cdbf78fd79738d38ecf314f9d8ee207558db2 
>   src/slave/slave.cpp 55e5264a61ce43970e177cf0eb27dc8a2657e261 
>   src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b 
>   src/tests/mesos.hpp e36e13843a36360d69a01f62356370bea588a337 
>   src/tests/slave_tests.cpp a1bd00cffe9de178b0b188c3556e479cd1f6d566 
> 
> Diff: https://reviews.apache.org/r/25525/diff/
> 
> 
> Testing
> ---
> 
> make check on localhost
> 
> 
> Thanks,
> 
> Cody Maloney
> 
>

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-10-21 Thread Cody Maloney


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

(Updated Oct. 22, 2014, 12:32 a.m.)


Review request for mesos, Adam B and Vinod Kone.


Changes
---

Rebase on master


Bugs: MESOS-1739
https://issues.apache.org/jira/browse/MESOS-1739


Repository: mesos-git


Description
---

Allows attributes and resources to be set to a superset of what they were 
previously on a slave restart.

Incorporates all comments from: 
https://issues.apache.org/jira/browse/MESOS-1739
and the former review request:
https://reviews.apache.org/r/25111/


Diffs (updated)
-

  src/Makefile.am c44a9ad47d6e1262949b9049f4ae25b049440d99 
  src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 
  src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
  src/common/slaveinfo_utils.hpp PRE-CREATION 
  src/common/slaveinfo_utils.cpp PRE-CREATION 
  src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223 
  src/master/hierarchical_allocator_process.hpp 
31dfb2cc86e2e74da43f05fbada10976ce65f3e4 
  src/master/master.hpp b1a2cd0f51f89d6dabbccaa67e0411fc55a4272f 
  src/master/master.cpp e70cdbf78fd79738d38ecf314f9d8ee207558db2 
  src/slave/slave.cpp 55e5264a61ce43970e177cf0eb27dc8a2657e261 
  src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b 
  src/tests/mesos.hpp e36e13843a36360d69a01f62356370bea588a337 
  src/tests/slave_tests.cpp a1bd00cffe9de178b0b188c3556e479cd1f6d566 

Diff: https://reviews.apache.org/r/25525/diff/


Testing
---

make check on localhost


Thanks,

Cody Maloney

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-10-21 Thread Mesos ReviewBot


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


Bad patch!

Reviews applied: [25525]

Failed command: ./support/apply-review.sh -n -r 25525

Error:
 --2014-10-22 00:11:36--  https://reviews.apache.org/r/25525/diff/raw/
Resolving reviews.apache.org (reviews.apache.org)... 140.211.11.74
Connecting to reviews.apache.org (reviews.apache.org)|140.211.11.74|:443... 
connected.
HTTP request sent, awaiting response... 200 OK
Length: 23834 (23K) [text/x-patch]
Saving to: '25525.patch'

 0K .. .. ... 100%  726K=0.03s

2014-10-22 00:11:36 (726 KB/s) - '25525.patch' saved [23834/23834]

error: patch failed: src/tests/slave_tests.cpp:1155
error: src/tests/slave_tests.cpp: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On Oct. 21, 2014, 11:55 p.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25525/
> ---
> 
> (Updated Oct. 21, 2014, 11:55 p.m.)
> 
> 
> Review request for mesos, Adam B and Vinod Kone.
> 
> 
> Bugs: MESOS-1739
> https://issues.apache.org/jira/browse/MESOS-1739
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Allows attributes and resources to be set to a superset of what they were 
> previously on a slave restart.
> 
> Incorporates all comments from: 
> https://issues.apache.org/jira/browse/MESOS-1739
> and the former review request:
> https://reviews.apache.org/r/25111/
> 
> 
> Diffs
> -
> 
>   src/Makefile.am c44a9ad47d6e1262949b9049f4ae25b049440d99 
>   src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 
>   src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
>   src/common/slaveinfo_utils.hpp PRE-CREATION 
>   src/common/slaveinfo_utils.cpp PRE-CREATION 
>   src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223 
>   src/master/hierarchical_allocator_process.hpp 
> 31dfb2cc86e2e74da43f05fbada10976ce65f3e4 
>   src/master/master.hpp 18898e9e1897b9d8cecd10d0ef7f25540cc9916d 
>   src/master/master.cpp be910d9f02405e25e5ad6ae3bf13d770a0c2b417 
>   src/slave/slave.cpp 7b5474ae898f369fc55670cf1dbbee684bc0ae4b 
>   src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b 
>   src/tests/mesos.hpp e40575c6d330cfa3fbfad2efcf601418c413b992 
>   src/tests/slave_tests.cpp 759670ad91c2f8afbd4b9dcb2f367f50482358e9 
> 
> Diff: https://reviews.apache.org/r/25525/diff/
> 
> 
> Testing
> ---
> 
> make check on localhost
> 
> 
> Thanks,
> 
> Cody Maloney
> 
>

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-10-21 Thread Cody Maloney



> On Oct. 15, 2014, 1:25 a.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 3146-3149
> > 
> >
> > Hmm. i don't think we want these semantics. Why not just fail if 
> > readmission failed?
> 
> Cody Maloney wrote:
> So if the registrar actually fails to readmit the slave, then the master 
> will go down on a LOG(FATAL). In the case where we couldn't readmit the slave 
> for some reason (Ex: the registrar thought the upgrade was incompatible, hit 
> the fall-through case in master/master.hpp), then the slave will be sent a 
> shutdown message and explicitly removed at that point.
> 
> So I think the semantics you want are there / the comment can just be 
> removed.
> 
> Vinod Kone wrote:
> i think "we just don't see..." threw me off. instead say that "readmit 
> will fail leading to slave shutdown.".
> 
> killing the comment is fine too.

Comment removed


> On Oct. 15, 2014, 1:25 a.m., Vinod Kone wrote:
> > src/common/slaveinfo_utils.cpp, lines 64-96
> > 
> >
> > Hmm. This is still hard to follow. 
> > 
> > Using variadic templated function here doesn't make it any simpler to 
> > read/follow. Infact it makes it harder because now I need to read 
> > understand what the template function does. I think explicitly checking 
> > each field (as suggested in the earlier comment) reads better.
> 
> Cody Maloney wrote:
> So I can do that, which gives:
> ```c++
>   if (!(newSlaveInfo.id() == oldSlaveInfo.id())) {
> return Error("id cannot be changed (old: " + 
> stringify(oldSlaveInfo.id()) +
>  " new: " + stringify(newSlaveInfo.id()) + ")");
>   }
> 
>   if (newSlaveInfo.hostname() != oldSlaveInfo.hostname()) {
> return Error("hostname cannot be changed (old: " +
>  stringify(oldSlaveInfo.hostname()) + " new: " +
>  stringify(newSlaveInfo.hostname()) + ")");
>   }
> 
>   if (newSlaveInfo.checkpoint() != oldSlaveInfo.checkpoint()) {
> return Error("checkpoint cannot be changed (old: " +
>  stringify(oldSlaveInfo.checkpoint()) + " new: " +
>  stringify(newSlaveInfo.checkpoint()) + ")");
>   }
> 
>   if (newSlaveInfo.port() != oldSlaveInfo.port()) {
> return Error("port cannot be changed (old: " +
>  stringify(oldSlaveInfo.port()) + " new: " +
>  stringify(newSlaveInfo.port()) + ")");
>   }
> ```
> 
> My problems with it are:
> 1. There is a lot of redundant code which needs to be mentally parsed. I 
> have to realize each of those is the same reading 3 lines of code before I 
> can recognize "oh, we're checking over and over again these two fields in the 
> different versions are the same, and mentally condense the code away"
> 2. How likely is it that you would actually notice all the new vs. old if 
> I got one of the error messages wrong? What about if I made any of a large 
> number of possible slight typos in one vs. the next? What if I compare two 
> different string fields? The compiler can't catch any of these or help me 
> with them.
> 
> 
> ```c++
> checkMembersUnchanged(newSlaveInfo, oldSlaveInfo,
> make_pair("SlaveID", &SlaveInfo::id),
> make_pair("hostname", &SlaveInfo::hostname),
> make_pair("checkpoint", &SlaveInfo::checkpoint),
> make_pair("port", &SlaveInfo::port));
> ```
> 
> Where when reading just that code, I see I'm giong to check that the 
> members are unchanged, probably between newSlaveInfo and oldSlaveInfo. That 
> the fields are "SlaveID", "hostname", "checkpoint", and "port". 
> 
> 
> The checkMembersUnchanged takes a little bit more time/effort to 
> recognize, esp. since it is c++11 variadic templates, recursion, and pointers 
> to member functions + calling pointers to member functions (Which is some of 
> the oddest syntax in C++). What is nice about it though is:
> 
> 1. The function though guarantees I call the same function on both old 
> and new / access the same member
> 2. Formats the error string consistently, gives me one place to update / 
> change it if needed
> 3. Makes one place for the ordering to be correct.
> 
> Vinod Kone wrote:
> If you look around the code base, we prefer approach 1 to 2. It's really 
> not that hard to follow that you are doing the same thing. Just adding a 
> comment on top would also help guide the users.
> 
> For readability, I would also suggest putting "(old: " on the next line 
> and "new: " on the next line.
> 
> With 2) my problem is that it is too esoteric, both the name and the 
> arguments.

Updated to have the checks written out.

The reason why the codebase has all the checks written out currently

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-10-21 Thread Cody Maloney



> On Oct. 21, 2014, 5:23 p.m., Vinod Kone wrote:
> > src/common/slaveinfo_utils.hpp, line 32
> > 
> >
> > Actually I think this would be better as "Option compatible()" 
> > instead of "Try isCompatible()", similar to how we do validateTask() 
> > in master.cpp. For places where you need to check whether nothing has 
> > changed, an equality check can be used.

Doing a seperate equality operation about doubles the computation at most 
callsites without needing to. It also complicates the callsite. Currently 2 out 
of 3 callers of the function need to check for the equality case. So they would 
have to gain more code to first check if the optional is set or not, if it is 
propogate up the error. And if not, then check for equality.

Changed the name to compatible().


- Cody


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


On Oct. 21, 2014, 11:55 p.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25525/
> ---
> 
> (Updated Oct. 21, 2014, 11:55 p.m.)
> 
> 
> Review request for mesos, Adam B and Vinod Kone.
> 
> 
> Bugs: MESOS-1739
> https://issues.apache.org/jira/browse/MESOS-1739
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Allows attributes and resources to be set to a superset of what they were 
> previously on a slave restart.
> 
> Incorporates all comments from: 
> https://issues.apache.org/jira/browse/MESOS-1739
> and the former review request:
> https://reviews.apache.org/r/25111/
> 
> 
> Diffs
> -
> 
>   src/Makefile.am c44a9ad47d6e1262949b9049f4ae25b049440d99 
>   src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 
>   src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
>   src/common/slaveinfo_utils.hpp PRE-CREATION 
>   src/common/slaveinfo_utils.cpp PRE-CREATION 
>   src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223 
>   src/master/hierarchical_allocator_process.hpp 
> 31dfb2cc86e2e74da43f05fbada10976ce65f3e4 
>   src/master/master.hpp 18898e9e1897b9d8cecd10d0ef7f25540cc9916d 
>   src/master/master.cpp be910d9f02405e25e5ad6ae3bf13d770a0c2b417 
>   src/slave/slave.cpp 7b5474ae898f369fc55670cf1dbbee684bc0ae4b 
>   src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b 
>   src/tests/mesos.hpp e40575c6d330cfa3fbfad2efcf601418c413b992 
>   src/tests/slave_tests.cpp 759670ad91c2f8afbd4b9dcb2f367f50482358e9 
> 
> Diff: https://reviews.apache.org/r/25525/diff/
> 
> 
> Testing
> ---
> 
> make check on localhost
> 
> 
> Thanks,
> 
> Cody Maloney
> 
>

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-10-21 Thread Cody Maloney


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

(Updated Oct. 21, 2014, 11:55 p.m.)


Review request for mesos, Adam B and Vinod Kone.


Changes
---

Update per reviewer comments


Bugs: MESOS-1739
https://issues.apache.org/jira/browse/MESOS-1739


Repository: mesos-git


Description
---

Allows attributes and resources to be set to a superset of what they were 
previously on a slave restart.

Incorporates all comments from: 
https://issues.apache.org/jira/browse/MESOS-1739
and the former review request:
https://reviews.apache.org/r/25111/


Diffs (updated)
-

  src/Makefile.am c44a9ad47d6e1262949b9049f4ae25b049440d99 
  src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 
  src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
  src/common/slaveinfo_utils.hpp PRE-CREATION 
  src/common/slaveinfo_utils.cpp PRE-CREATION 
  src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223 
  src/master/hierarchical_allocator_process.hpp 
31dfb2cc86e2e74da43f05fbada10976ce65f3e4 
  src/master/master.hpp 18898e9e1897b9d8cecd10d0ef7f25540cc9916d 
  src/master/master.cpp be910d9f02405e25e5ad6ae3bf13d770a0c2b417 
  src/slave/slave.cpp 7b5474ae898f369fc55670cf1dbbee684bc0ae4b 
  src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b 
  src/tests/mesos.hpp e40575c6d330cfa3fbfad2efcf601418c413b992 
  src/tests/slave_tests.cpp 759670ad91c2f8afbd4b9dcb2f367f50482358e9 

Diff: https://reviews.apache.org/r/25525/diff/


Testing
---

make check on localhost


Thanks,

Cody Maloney

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-10-21 Thread Vinod Kone



> On Oct. 15, 2014, 1:25 a.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 3146-3149
> > 
> >
> > Hmm. i don't think we want these semantics. Why not just fail if 
> > readmission failed?
> 
> Cody Maloney wrote:
> So if the registrar actually fails to readmit the slave, then the master 
> will go down on a LOG(FATAL). In the case where we couldn't readmit the slave 
> for some reason (Ex: the registrar thought the upgrade was incompatible, hit 
> the fall-through case in master/master.hpp), then the slave will be sent a 
> shutdown message and explicitly removed at that point.
> 
> So I think the semantics you want are there / the comment can just be 
> removed.

i think "we just don't see..." threw me off. instead say that "readmit will 
fail leading to slave shutdown.".

killing the comment is fine too.


> On Oct. 15, 2014, 1:25 a.m., Vinod Kone wrote:
> > src/common/slaveinfo_utils.cpp, lines 64-96
> > 
> >
> > Hmm. This is still hard to follow. 
> > 
> > Using variadic templated function here doesn't make it any simpler to 
> > read/follow. Infact it makes it harder because now I need to read 
> > understand what the template function does. I think explicitly checking 
> > each field (as suggested in the earlier comment) reads better.
> 
> Cody Maloney wrote:
> So I can do that, which gives:
> ```c++
>   if (!(newSlaveInfo.id() == oldSlaveInfo.id())) {
> return Error("id cannot be changed (old: " + 
> stringify(oldSlaveInfo.id()) +
>  " new: " + stringify(newSlaveInfo.id()) + ")");
>   }
> 
>   if (newSlaveInfo.hostname() != oldSlaveInfo.hostname()) {
> return Error("hostname cannot be changed (old: " +
>  stringify(oldSlaveInfo.hostname()) + " new: " +
>  stringify(newSlaveInfo.hostname()) + ")");
>   }
> 
>   if (newSlaveInfo.checkpoint() != oldSlaveInfo.checkpoint()) {
> return Error("checkpoint cannot be changed (old: " +
>  stringify(oldSlaveInfo.checkpoint()) + " new: " +
>  stringify(newSlaveInfo.checkpoint()) + ")");
>   }
> 
>   if (newSlaveInfo.port() != oldSlaveInfo.port()) {
> return Error("port cannot be changed (old: " +
>  stringify(oldSlaveInfo.port()) + " new: " +
>  stringify(newSlaveInfo.port()) + ")");
>   }
> ```
> 
> My problems with it are:
> 1. There is a lot of redundant code which needs to be mentally parsed. I 
> have to realize each of those is the same reading 3 lines of code before I 
> can recognize "oh, we're checking over and over again these two fields in the 
> different versions are the same, and mentally condense the code away"
> 2. How likely is it that you would actually notice all the new vs. old if 
> I got one of the error messages wrong? What about if I made any of a large 
> number of possible slight typos in one vs. the next? What if I compare two 
> different string fields? The compiler can't catch any of these or help me 
> with them.
> 
> 
> ```c++
> checkMembersUnchanged(newSlaveInfo, oldSlaveInfo,
> make_pair("SlaveID", &SlaveInfo::id),
> make_pair("hostname", &SlaveInfo::hostname),
> make_pair("checkpoint", &SlaveInfo::checkpoint),
> make_pair("port", &SlaveInfo::port));
> ```
> 
> Where when reading just that code, I see I'm giong to check that the 
> members are unchanged, probably between newSlaveInfo and oldSlaveInfo. That 
> the fields are "SlaveID", "hostname", "checkpoint", and "port". 
> 
> 
> The checkMembersUnchanged takes a little bit more time/effort to 
> recognize, esp. since it is c++11 variadic templates, recursion, and pointers 
> to member functions + calling pointers to member functions (Which is some of 
> the oddest syntax in C++). What is nice about it though is:
> 
> 1. The function though guarantees I call the same function on both old 
> and new / access the same member
> 2. Formats the error string consistently, gives me one place to update / 
> change it if needed
> 3. Makes one place for the ordering to be correct.

If you look around the code base, we prefer approach 1 to 2. It's really not 
that hard to follow that you are doing the same thing. Just adding a comment on 
top would also help guide the users.

For readability, I would also suggest putting "(old: " on the next line and 
"new: " on the next line.

With 2) my problem is that it is too esoteric, both the name and the arguments.


> On Oct. 15, 2014, 1:25 a.m., Vinod Kone wrote:
> > src/master/master.hpp, line 1308
> > 
> >
> > This should be CHECK_SOME(compatible).
> 
> Cody M

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-10-21 Thread Vinod Kone


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



src/common/slaveinfo_utils.hpp


Actually I think this would be better as "Option compatible()" 
instead of "Try isCompatible()", similar to how we do validateTask() in 
master.cpp. For places where you need to check whether nothing has changed, an 
equality check can be used.


- Vinod Kone


On Oct. 15, 2014, 7:30 p.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25525/
> ---
> 
> (Updated Oct. 15, 2014, 7:30 p.m.)
> 
> 
> Review request for mesos, Adam B and Vinod Kone.
> 
> 
> Bugs: MESOS-1739
> https://issues.apache.org/jira/browse/MESOS-1739
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Allows attributes and resources to be set to a superset of what they were 
> previously on a slave restart.
> 
> Incorporates all comments from: 
> https://issues.apache.org/jira/browse/MESOS-1739
> and the former review request:
> https://reviews.apache.org/r/25111/
> 
> 
> Diffs
> -
> 
>   src/Makefile.am d503c8df73cda15a9d59254e8265e4a5d0e003a4 
>   src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 
>   src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
>   src/common/slaveinfo_utils.hpp PRE-CREATION 
>   src/common/slaveinfo_utils.cpp PRE-CREATION 
>   src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223 
>   src/master/hierarchical_allocator_process.hpp 
> 31dfb2cc86e2e74da43f05fbada10976ce65f3e4 
>   src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205 
>   src/master/master.cpp 0a5c9a374062a241c90ea238725fbb8dd2408ef4 
>   src/slave/slave.cpp 0e342ed35e3db3b68f9f32b6cf4ace23e4a4db38 
>   src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b 
>   src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 
>   src/tests/slave_tests.cpp f585bdd20ae1af466f2c1b4d85331ac67451552f 
> 
> Diff: https://reviews.apache.org/r/25525/diff/
> 
> 
> Testing
> ---
> 
> make check on localhost
> 
> 
> Thanks,
> 
> Cody Maloney
> 
>

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-10-15 Thread Cody Maloney



> On Oct. 16, 2014, 2:50 a.m., Cody Maloney wrote:
> > Bad patch!
> > 
> > Reviews applied: [25525]
> > 
> > Failed command: ['./support/appy-review.sh', '-r', '25525']
> > 
> > Error:
> >  -r: ./support/appy-review.sh: No such file or directory

Ignore this. I was testing out some changes to the review checking script, 
broke things.


- Cody


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


On Oct. 15, 2014, 7:30 p.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25525/
> ---
> 
> (Updated Oct. 15, 2014, 7:30 p.m.)
> 
> 
> Review request for mesos, Adam B and Vinod Kone.
> 
> 
> Bugs: MESOS-1739
> https://issues.apache.org/jira/browse/MESOS-1739
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Allows attributes and resources to be set to a superset of what they were 
> previously on a slave restart.
> 
> Incorporates all comments from: 
> https://issues.apache.org/jira/browse/MESOS-1739
> and the former review request:
> https://reviews.apache.org/r/25111/
> 
> 
> Diffs
> -
> 
>   src/Makefile.am d503c8df73cda15a9d59254e8265e4a5d0e003a4 
>   src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 
>   src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
>   src/common/slaveinfo_utils.hpp PRE-CREATION 
>   src/common/slaveinfo_utils.cpp PRE-CREATION 
>   src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223 
>   src/master/hierarchical_allocator_process.hpp 
> 31dfb2cc86e2e74da43f05fbada10976ce65f3e4 
>   src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205 
>   src/master/master.cpp 0a5c9a374062a241c90ea238725fbb8dd2408ef4 
>   src/slave/slave.cpp 0e342ed35e3db3b68f9f32b6cf4ace23e4a4db38 
>   src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b 
>   src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 
>   src/tests/slave_tests.cpp f585bdd20ae1af466f2c1b4d85331ac67451552f 
> 
> Diff: https://reviews.apache.org/r/25525/diff/
> 
> 
> Testing
> ---
> 
> make check on localhost
> 
> 
> Thanks,
> 
> Cody Maloney
> 
>

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-10-15 Thread Cody Maloney


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


Bad patch!

Reviews applied: [25525]

Failed command: ['./support/appy-review.sh', '-r', '25525']

Error:
 -r: ./support/appy-review.sh: No such file or directory

- Cody Maloney


On Oct. 15, 2014, 7:30 p.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25525/
> ---
> 
> (Updated Oct. 15, 2014, 7:30 p.m.)
> 
> 
> Review request for mesos, Adam B and Vinod Kone.
> 
> 
> Bugs: MESOS-1739
> https://issues.apache.org/jira/browse/MESOS-1739
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Allows attributes and resources to be set to a superset of what they were 
> previously on a slave restart.
> 
> Incorporates all comments from: 
> https://issues.apache.org/jira/browse/MESOS-1739
> and the former review request:
> https://reviews.apache.org/r/25111/
> 
> 
> Diffs
> -
> 
>   src/Makefile.am d503c8df73cda15a9d59254e8265e4a5d0e003a4 
>   src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 
>   src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
>   src/common/slaveinfo_utils.hpp PRE-CREATION 
>   src/common/slaveinfo_utils.cpp PRE-CREATION 
>   src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223 
>   src/master/hierarchical_allocator_process.hpp 
> 31dfb2cc86e2e74da43f05fbada10976ce65f3e4 
>   src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205 
>   src/master/master.cpp 0a5c9a374062a241c90ea238725fbb8dd2408ef4 
>   src/slave/slave.cpp 0e342ed35e3db3b68f9f32b6cf4ace23e4a4db38 
>   src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b 
>   src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 
>   src/tests/slave_tests.cpp f585bdd20ae1af466f2c1b4d85331ac67451552f 
> 
> Diff: https://reviews.apache.org/r/25525/diff/
> 
> 
> Testing
> ---
> 
> make check on localhost
> 
> 
> Thanks,
> 
> Cody Maloney
> 
>

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-10-15 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [25525]

All tests passed.

- Mesos ReviewBot


On Oct. 15, 2014, 7:30 p.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25525/
> ---
> 
> (Updated Oct. 15, 2014, 7:30 p.m.)
> 
> 
> Review request for mesos, Adam B and Vinod Kone.
> 
> 
> Bugs: MESOS-1739
> https://issues.apache.org/jira/browse/MESOS-1739
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Allows attributes and resources to be set to a superset of what they were 
> previously on a slave restart.
> 
> Incorporates all comments from: 
> https://issues.apache.org/jira/browse/MESOS-1739
> and the former review request:
> https://reviews.apache.org/r/25111/
> 
> 
> Diffs
> -
> 
>   src/Makefile.am d503c8df73cda15a9d59254e8265e4a5d0e003a4 
>   src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 
>   src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
>   src/common/slaveinfo_utils.hpp PRE-CREATION 
>   src/common/slaveinfo_utils.cpp PRE-CREATION 
>   src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223 
>   src/master/hierarchical_allocator_process.hpp 
> 31dfb2cc86e2e74da43f05fbada10976ce65f3e4 
>   src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205 
>   src/master/master.cpp 0a5c9a374062a241c90ea238725fbb8dd2408ef4 
>   src/slave/slave.cpp 0e342ed35e3db3b68f9f32b6cf4ace23e4a4db38 
>   src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b 
>   src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 
>   src/tests/slave_tests.cpp f585bdd20ae1af466f2c1b4d85331ac67451552f 
> 
> Diff: https://reviews.apache.org/r/25525/diff/
> 
> 
> Testing
> ---
> 
> make check on localhost
> 
> 
> Thanks,
> 
> Cody Maloney
> 
>

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-10-15 Thread Cody Maloney


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

(Updated Oct. 15, 2014, 7:30 p.m.)


Review request for mesos, Adam B and Vinod Kone.


Changes
---

Fixed build error


Bugs: MESOS-1739
https://issues.apache.org/jira/browse/MESOS-1739


Repository: mesos-git


Description
---

Allows attributes and resources to be set to a superset of what they were 
previously on a slave restart.

Incorporates all comments from: 
https://issues.apache.org/jira/browse/MESOS-1739
and the former review request:
https://reviews.apache.org/r/25111/


Diffs (updated)
-

  src/Makefile.am d503c8df73cda15a9d59254e8265e4a5d0e003a4 
  src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 
  src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
  src/common/slaveinfo_utils.hpp PRE-CREATION 
  src/common/slaveinfo_utils.cpp PRE-CREATION 
  src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223 
  src/master/hierarchical_allocator_process.hpp 
31dfb2cc86e2e74da43f05fbada10976ce65f3e4 
  src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205 
  src/master/master.cpp 0a5c9a374062a241c90ea238725fbb8dd2408ef4 
  src/slave/slave.cpp 0e342ed35e3db3b68f9f32b6cf4ace23e4a4db38 
  src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b 
  src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 
  src/tests/slave_tests.cpp f585bdd20ae1af466f2c1b4d85331ac67451552f 

Diff: https://reviews.apache.org/r/25525/diff/


Testing
---

make check on localhost


Thanks,

Cody Maloney

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-10-14 Thread Cody Maloney


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

(Updated Oct. 15, 2014, 3:13 a.m.)


Review request for mesos, Adam B and Vinod Kone.


Changes
---

Missed a couple comments


Bugs: MESOS-1739
https://issues.apache.org/jira/browse/MESOS-1739


Repository: mesos-git


Description
---

Allows attributes and resources to be set to a superset of what they were 
previously on a slave restart.

Incorporates all comments from: 
https://issues.apache.org/jira/browse/MESOS-1739
and the former review request:
https://reviews.apache.org/r/25111/


Diffs (updated)
-

  src/Makefile.am d503c8df73cda15a9d59254e8265e4a5d0e003a4 
  src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 
  src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
  src/common/slaveinfo_utils.hpp PRE-CREATION 
  src/common/slaveinfo_utils.cpp PRE-CREATION 
  src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223 
  src/master/hierarchical_allocator_process.hpp 
31dfb2cc86e2e74da43f05fbada10976ce65f3e4 
  src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205 
  src/master/master.cpp 1b1ce0de3065ced45890777d42c69ef1091f5699 
  src/slave/slave.cpp 0e342ed35e3db3b68f9f32b6cf4ace23e4a4db38 
  src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b 
  src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 
  src/tests/slave_tests.cpp f585bdd20ae1af466f2c1b4d85331ac67451552f 

Diff: https://reviews.apache.org/r/25525/diff/


Testing
---

make check on localhost


Thanks,

Cody Maloney

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-10-14 Thread Cody Maloney



- Cody


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


On Oct. 15, 2014, 3:13 a.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25525/
> ---
> 
> (Updated Oct. 15, 2014, 3:13 a.m.)
> 
> 
> Review request for mesos, Adam B and Vinod Kone.
> 
> 
> Bugs: MESOS-1739
> https://issues.apache.org/jira/browse/MESOS-1739
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Allows attributes and resources to be set to a superset of what they were 
> previously on a slave restart.
> 
> Incorporates all comments from: 
> https://issues.apache.org/jira/browse/MESOS-1739
> and the former review request:
> https://reviews.apache.org/r/25111/
> 
> 
> Diffs
> -
> 
>   src/Makefile.am d503c8df73cda15a9d59254e8265e4a5d0e003a4 
>   src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 
>   src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
>   src/common/slaveinfo_utils.hpp PRE-CREATION 
>   src/common/slaveinfo_utils.cpp PRE-CREATION 
>   src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223 
>   src/master/hierarchical_allocator_process.hpp 
> 31dfb2cc86e2e74da43f05fbada10976ce65f3e4 
>   src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205 
>   src/master/master.cpp 1b1ce0de3065ced45890777d42c69ef1091f5699 
>   src/slave/slave.cpp 0e342ed35e3db3b68f9f32b6cf4ace23e4a4db38 
>   src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b 
>   src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 
>   src/tests/slave_tests.cpp f585bdd20ae1af466f2c1b4d85331ac67451552f 
> 
> Diff: https://reviews.apache.org/r/25525/diff/
> 
> 
> Testing
> ---
> 
> make check on localhost
> 
> 
> Thanks,
> 
> Cody Maloney
> 
>

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-10-14 Thread Cody Maloney

> On Oct. 15, 2014, 1:25 a.m., Vinod Kone wrote:
> > src/master/master.hpp, line 1308
> > 
> >
> > This should be CHECK_SOME(compatible).

True, a incompatible change shouldn't get that far because there are advance 
checks, but practically the registrar will do the right thing / recover in the 
case of a compatibility error here. CHECK_SOME() means that we will always 
fatally exit and kill the master, which seems worse to me.

> On Oct. 15, 2014, 1:25 a.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 3100-3102
> > 
> >
> > ```
> > LOG(WARNING) << "Slave " << *slave << " attempted to re-register with 
> > incompatible info: " 
> >  << compatible.error() << "; shutting it down"
> > ```
> > 
> > Also, maybe we don't need to explicitly 'removeSlave(slave)' here? If a 
> > compatible slave is not re-registered within timeout, master will 
> > automatically remove it.

Updated the message. I would rather explicitly tell the slave to shutdown than 
wait, otherwise the slave will continue trying to reregister with the master, 
the master will keep saying no, until the timeout at which point we will tell 
the slave to shutdown when it tries to re-authenticate with the message "Slave 
attempted to re-register after removal".

Waiting gives the operator worse information, and makes it a longer loop for 
getting the slave back into the cluster, than giving the notification, and it 
really isn't much (or very complex) code.

> On Oct. 15, 2014, 1:25 a.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 3146-3149
> > 
> >
> > Hmm. i don't think we want these semantics. Why not just fail if 
> > readmission failed?

So if the registrar actually fails to readmit the slave, then the master will 
go down on a LOG(FATAL). In the case where we couldn't readmit the slave for 
some reason (Ex: the registrar thought the upgrade was incompatible, hit the 
fall-through case in master/master.hpp), then the slave will be sent a shutdown 
message and explicitly removed at that point.

So I think the semantics you want are there / the comment can just be removed.

> On Oct. 15, 2014, 1:25 a.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 3169-3171
> > 
> >
> > This seems very unintuitve for users/operators. Lets fix this in this 
> > patch rather than doing a TODO.
> > 
> > 
> > Also, AFAICT, the changes are not lost because both the registrar and 
> > the allocator are informed about the new slave info. Am I missing something?

Note this isn't a fall through from the above code. All the above code exits. 
At this point, we haven't talked to the registrar or allocator. All that has 
happened since reciept of the message is authentication, checking that the 
slave hasn't already been removed, and checking that the master doesn't already 
know about the slave (getSlave reutrns null).

So this can be triggered by a slave reregistering, and that first registration 
being sent to the Registrar.

While that registration is happening, the same slave sends another 
reregistration message, this time with updated SlaveInfo. The re-registration 
check will be hit, and the updated SlaveInfo will be discarded. per the check.

The solutions I can see:
1) Send a message back to the slave that we already got a reregistration with 
the last registration's SlaveInfo, and if it doesn't want that one it needs to 
update it's registration after that one has completed
2) Make it so we can queue multiple in-flight reregistrations for a single 
slave (No idea if this will break anything / would work if you just removed the 
check). I worry a little with this option that there could be a lot of 
reregistrations in flight at the same time to the master and it could get 
overwhelmed doing extra work where before there was an early exit.

> On Oct. 15, 2014, 1:25 a.m., Vinod Kone wrote:
> > src/common/slaveinfo_utils.cpp, lines 64-96
> > 
> >
> > Hmm. This is still hard to follow. 
> > 
> > Using variadic templated function here doesn't make it any simpler to 
> > read/follow. Infact it makes it harder because now I need to read 
> > understand what the template function does. I think explicitly checking 
> > each field (as suggested in the earlier comment) reads better.

So I can do that, which gives:
```c++
  if (!(newSlaveInfo.id() == oldSlaveInfo.id())) {
return Error("id cannot be changed (old: " + stringify(oldSlaveInfo.id()) +
 " new: " + stringify(newSlaveInfo.id()) + ")");
  }

  if (newSlaveInfo.hostname() != oldSlaveInfo.hostname()) {
return Error("hostname cannot be

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-10-14 Thread Cody Maloney


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

(Updated Oct. 15, 2014, 3:06 a.m.)


Review request for mesos, Adam B and Vinod Kone.


Changes
---

Update to address review comments.


Bugs: MESOS-1739
https://issues.apache.org/jira/browse/MESOS-1739


Repository: mesos-git


Description
---

Allows attributes and resources to be set to a superset of what they were 
previously on a slave restart.

Incorporates all comments from: 
https://issues.apache.org/jira/browse/MESOS-1739
and the former review request:
https://reviews.apache.org/r/25111/


Diffs (updated)
-

  src/Makefile.am d503c8df73cda15a9d59254e8265e4a5d0e003a4 
  src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 
  src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
  src/common/slaveinfo_utils.hpp PRE-CREATION 
  src/common/slaveinfo_utils.cpp PRE-CREATION 
  src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223 
  src/master/hierarchical_allocator_process.hpp 
31dfb2cc86e2e74da43f05fbada10976ce65f3e4 
  src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205 
  src/master/master.cpp 1b1ce0de3065ced45890777d42c69ef1091f5699 
  src/slave/slave.cpp 0e342ed35e3db3b68f9f32b6cf4ace23e4a4db38 
  src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b 
  src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 
  src/tests/slave_tests.cpp f585bdd20ae1af466f2c1b4d85331ac67451552f 

Diff: https://reviews.apache.org/r/25525/diff/


Testing
---

make check on localhost


Thanks,

Cody Maloney

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-10-14 Thread Vinod Kone

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

src/common/slaveinfo_utils.hpp

s/legal/compatible/

Also mention when this returns a false vs error.

src/common/slaveinfo_utils.cpp

Hmm. This is still hard to follow. 

Using variadic templated function here doesn't make it any simpler to 
read/follow. Infact it makes it harder because now I need to read understand 
what the template function does. I think explicitly checking each field (as 
suggested in the earlier comment) reads better.

src/master/hierarchical_allocator_process.hpp

Instead of adding a 'update()' method on Slave struct, just manipulate the 
resources here.

Also, you want to CHECK that the new resources are more than the old 
resources.

src/master/master.hpp

Can you rephrase this? It's hard to follow.

src/master/master.hpp

This should be CHECK_SOME(compatible).

src/master/master.cpp

```
LOG(WARNING) << "Slave " << *slave << " attempted to re-register with 
incompatible info: " 
 << compatible.error() << "; shutting it down"
```

Also, maybe we don't need to explicitly 'removeSlave(slave)' here? If a 
compatible slave is not re-registered within timeout, master will automatically 
remove it.

src/master/master.cpp

include "compatible.error()" in the message.

src/master/master.cpp

s/Replicate the updated attribute to the other masters/Readmit the slave 
with updated slave info/

Also, period at the end of comments please.

src/master/master.cpp

Hmm. i don't think we want these semantics. Why not just fail if 
readmission failed?

src/master/master.cpp

s/__reregisterSlaveExistingMaster/__reregisterUpdatedSlave/

src/master/master.cpp

This seems very unintuitve for users/operators. Lets fix this in this patch 
rather than doing a TODO.

Also, AFAICT, the changes are not lost because both the registrar and the 
allocator are informed about the new slave info. Am I missing something?

src/master/master.cpp

2 blank lines.

src/master/master.cpp

2 blank lines.

src/slave/slave.cpp

We can kill the NOTE above because now we set the 'id' field because 'info' 
is going to be the updated slave info that slave uses.

src/slave/slave.cpp

End statements with a period.

s/slaveInfo/slave info/

src/slave/slave.cpp

Consider:

```
if (flags.checkpoint && compatible.get()) {
  const string& path = paths::getSlaveInfoPath(metaDir, info.id()); 

  LOG(INFO) << "Checkpointing updated SlaveInfo to ' << path << "'";
  CHECK_SOME(state::checkpoint(path, info));
}
```

src/tests/attributes_tests.cpp

2 blank lines between outer elements.

src/tests/mesos.hpp

2 blank lines.

src/tests/mesos.hpp

2 blank lines.

src/tests/slave_tests.cpp

2 blank lines.

src/tests/slave_tests.cpp

Any particular reason you want to explicitly wait for slave registration? 
FWIW, the AWAIT on offers below will guarantee that slave has registered.

src/tests/slave_tests.cpp

s/new_offers/newOffers/

we use camel case for variables in the code base as much as possible.

src/tests/slave_tests.cpp

s/recovered_slave/recoveredSlave/

src/tests/slave_tests.cpp

ditto. Why explicitly wait for recovery? Reception of new offers guarantees 
that.

- Vinod Kone

On Oct. 10, 2014, 6:49 p.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visi

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-10-10 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [25525]

All tests passed.

- Mesos ReviewBot


On Oct. 10, 2014, 6:49 p.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25525/
> ---
> 
> (Updated Oct. 10, 2014, 6:49 p.m.)
> 
> 
> Review request for mesos, Adam B and Vinod Kone.
> 
> 
> Bugs: MESOS-1739
> https://issues.apache.org/jira/browse/MESOS-1739
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Allows attributes and resources to be set to a superset of what they were 
> previously on a slave restart.
> 
> Incorporates all comments from: 
> https://issues.apache.org/jira/browse/MESOS-1739
> and the former review request:
> https://reviews.apache.org/r/25111/
> 
> 
> Diffs
> -
> 
>   src/Makefile.am d503c8df73cda15a9d59254e8265e4a5d0e003a4 
>   src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 
>   src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
>   src/common/slaveinfo_utils.hpp PRE-CREATION 
>   src/common/slaveinfo_utils.cpp PRE-CREATION 
>   src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223 
>   src/master/hierarchical_allocator_process.hpp 
> 31dfb2cc86e2e74da43f05fbada10976ce65f3e4 
>   src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205 
>   src/master/master.cpp cb46cec0674b3aa031450c5b4f48f4f8bb92767d 
>   src/slave/slave.cpp cb3759993f863590cb1545c73072feb0331aa6c9 
>   src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b 
>   src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 
>   src/tests/slave_tests.cpp 69be28f6e82b99e23424bd2be8294f715d8040d4 
> 
> Diff: https://reviews.apache.org/r/25525/diff/
> 
> 
> Testing
> ---
> 
> make check on localhost
> 
> 
> Thanks,
> 
> Cody Maloney
> 
>

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-10-10 Thread Cody Maloney


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

(Updated Oct. 10, 2014, 6:49 p.m.)


Review request for mesos, Adam B and Vinod Kone.


Changes
---

Fix style issues


Bugs: MESOS-1739
https://issues.apache.org/jira/browse/MESOS-1739


Repository: mesos-git


Description
---

Allows attributes and resources to be set to a superset of what they were 
previously on a slave restart.

Incorporates all comments from: 
https://issues.apache.org/jira/browse/MESOS-1739
and the former review request:
https://reviews.apache.org/r/25111/


Diffs (updated)
-

  src/Makefile.am d503c8df73cda15a9d59254e8265e4a5d0e003a4 
  src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 
  src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
  src/common/slaveinfo_utils.hpp PRE-CREATION 
  src/common/slaveinfo_utils.cpp PRE-CREATION 
  src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223 
  src/master/hierarchical_allocator_process.hpp 
31dfb2cc86e2e74da43f05fbada10976ce65f3e4 
  src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205 
  src/master/master.cpp cb46cec0674b3aa031450c5b4f48f4f8bb92767d 
  src/slave/slave.cpp cb3759993f863590cb1545c73072feb0331aa6c9 
  src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b 
  src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 
  src/tests/slave_tests.cpp 69be28f6e82b99e23424bd2be8294f715d8040d4 

Diff: https://reviews.apache.org/r/25525/diff/


Testing
---

make check on localhost


Thanks,

Cody Maloney

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-10-10 Thread Cody Maloney


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



src/common/attributes.cpp


{ on next line



src/common/attributes.cpp


{ on next line



src/master/hierarchical_allocator_process.hpp


{} on new line



src/master/hierarchical_allocator_process.hpp


Add '.' to comment


- Cody Maloney


On Oct. 10, 2014, 2:29 a.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25525/
> ---
> 
> (Updated Oct. 10, 2014, 2:29 a.m.)
> 
> 
> Review request for mesos, Adam B and Vinod Kone.
> 
> 
> Bugs: MESOS-1739
> https://issues.apache.org/jira/browse/MESOS-1739
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Allows attributes and resources to be set to a superset of what they were 
> previously on a slave restart.
> 
> Incorporates all comments from: 
> https://issues.apache.org/jira/browse/MESOS-1739
> and the former review request:
> https://reviews.apache.org/r/25111/
> 
> 
> Diffs
> -
> 
>   src/Makefile.am d503c8df73cda15a9d59254e8265e4a5d0e003a4 
>   src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 
>   src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
>   src/common/slaveinfo_utils.hpp PRE-CREATION 
>   src/common/slaveinfo_utils.cpp PRE-CREATION 
>   src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223 
>   src/master/hierarchical_allocator_process.hpp 
> 31dfb2cc86e2e74da43f05fbada10976ce65f3e4 
>   src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205 
>   src/master/master.cpp cb46cec0674b3aa031450c5b4f48f4f8bb92767d 
>   src/slave/slave.cpp cb3759993f863590cb1545c73072feb0331aa6c9 
>   src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b 
>   src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 
>   src/tests/slave_tests.cpp 69be28f6e82b99e23424bd2be8294f715d8040d4 
> 
> Diff: https://reviews.apache.org/r/25525/diff/
> 
> 
> Testing
> ---
> 
> make check on localhost
> 
> 
> Thanks,
> 
> Cody Maloney
> 
>

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-10-09 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [25525]

All tests passed.

- Mesos ReviewBot


On Oct. 10, 2014, 2:29 a.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25525/
> ---
> 
> (Updated Oct. 10, 2014, 2:29 a.m.)
> 
> 
> Review request for mesos, Adam B and Vinod Kone.
> 
> 
> Bugs: MESOS-1739
> https://issues.apache.org/jira/browse/MESOS-1739
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Allows attributes and resources to be set to a superset of what they were 
> previously on a slave restart.
> 
> Incorporates all comments from: 
> https://issues.apache.org/jira/browse/MESOS-1739
> and the former review request:
> https://reviews.apache.org/r/25111/
> 
> 
> Diffs
> -
> 
>   src/Makefile.am d503c8df73cda15a9d59254e8265e4a5d0e003a4 
>   src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 
>   src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
>   src/common/slaveinfo_utils.hpp PRE-CREATION 
>   src/common/slaveinfo_utils.cpp PRE-CREATION 
>   src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223 
>   src/master/hierarchical_allocator_process.hpp 
> 31dfb2cc86e2e74da43f05fbada10976ce65f3e4 
>   src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205 
>   src/master/master.cpp cb46cec0674b3aa031450c5b4f48f4f8bb92767d 
>   src/slave/slave.cpp cb3759993f863590cb1545c73072feb0331aa6c9 
>   src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b 
>   src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 
>   src/tests/slave_tests.cpp 69be28f6e82b99e23424bd2be8294f715d8040d4 
> 
> Diff: https://reviews.apache.org/r/25525/diff/
> 
> 
> Testing
> ---
> 
> make check on localhost
> 
> 
> Thanks,
> 
> Cody Maloney
> 
>

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-10-09 Thread Cody Maloney


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

(Updated Oct. 10, 2014, 2:29 a.m.)


Review request for mesos, Adam B and Vinod Kone.


Bugs: MESOS-1739
https://issues.apache.org/jira/browse/MESOS-1739


Repository: mesos-git


Description
---

Allows attributes and resources to be set to a superset of what they were 
previously on a slave restart.

Incorporates all comments from: 
https://issues.apache.org/jira/browse/MESOS-1739
and the former review request:
https://reviews.apache.org/r/25111/


Diffs
-

  src/Makefile.am d503c8df73cda15a9d59254e8265e4a5d0e003a4 
  src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 
  src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
  src/common/slaveinfo_utils.hpp PRE-CREATION 
  src/common/slaveinfo_utils.cpp PRE-CREATION 
  src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223 
  src/master/hierarchical_allocator_process.hpp 
31dfb2cc86e2e74da43f05fbada10976ce65f3e4 
  src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205 
  src/master/master.cpp cb46cec0674b3aa031450c5b4f48f4f8bb92767d 
  src/slave/slave.cpp cb3759993f863590cb1545c73072feb0331aa6c9 
  src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b 
  src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 
  src/tests/slave_tests.cpp 69be28f6e82b99e23424bd2be8294f715d8040d4 

Diff: https://reviews.apache.org/r/25525/diff/


Testing
---

make check on localhost


Thanks,

Cody Maloney

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-10-09 Thread Cody Maloney


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

(Updated Oct. 10, 2014, 2:29 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, Patrick Reilly, and Vinod 
Kone.


Changes
---

Add slaveUpdated to Allocator API, per comments on the design document.
Rebase on top of latest mesos changes.


Bugs: MESOS-1739
https://issues.apache.org/jira/browse/MESOS-1739


Repository: mesos-git


Description
---

Allows attributes and resources to be set to a superset of what they were 
previously on a slave restart.

Incorporates all comments from: 
https://issues.apache.org/jira/browse/MESOS-1739
and the former review request:
https://reviews.apache.org/r/25111/


Diffs (updated)
-

  src/Makefile.am d503c8df73cda15a9d59254e8265e4a5d0e003a4 
  src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 
  src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
  src/common/slaveinfo_utils.hpp PRE-CREATION 
  src/common/slaveinfo_utils.cpp PRE-CREATION 
  src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223 
  src/master/hierarchical_allocator_process.hpp 
31dfb2cc86e2e74da43f05fbada10976ce65f3e4 
  src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205 
  src/master/master.cpp cb46cec0674b3aa031450c5b4f48f4f8bb92767d 
  src/slave/slave.cpp cb3759993f863590cb1545c73072feb0331aa6c9 
  src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b 
  src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 
  src/tests/slave_tests.cpp 69be28f6e82b99e23424bd2be8294f715d8040d4 

Diff: https://reviews.apache.org/r/25525/diff/


Testing
---

make check on localhost


Thanks,

Cody Maloney

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-09-30 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [25525]

All tests passed.

- Mesos ReviewBot


On Sept. 30, 2014, 6:05 p.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25525/
> ---
> 
> (Updated Sept. 30, 2014, 6:05 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Patrick Reilly, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-1739
> https://issues.apache.org/jira/browse/MESOS-1739
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Allows attributes and resources to be set to a superset of what they were 
> previously on a slave restart.
> 
> Incorporates all comments from: 
> https://issues.apache.org/jira/browse/MESOS-1739
> and the former review request:
> https://reviews.apache.org/r/25111/
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 27c42dfde45a449750132e416b4eaf776f8c5e3b 
>   src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 
>   src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
>   src/common/slaveinfo_utils.hpp PRE-CREATION 
>   src/common/slaveinfo_utils.cpp PRE-CREATION 
>   src/master/master.hpp d6380199421840aa17d4ce2725dcbcf4a11ce85f 
>   src/master/master.cpp a60308f912a1ed81ecd51c677461a8f591d9eb8e 
>   src/slave/slave.cpp e56dcbd80114730949a0d4b553470802a4d38281 
>   src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b 
>   src/tests/slave_tests.cpp 69be28f6e82b99e23424bd2be8294f715d8040d4 
> 
> Diff: https://reviews.apache.org/r/25525/diff/
> 
> 
> Testing
> ---
> 
> make check on localhost
> 
> 
> Thanks,
> 
> Cody Maloney
> 
>

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-09-30 Thread Cody Maloney


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

(Updated Sept. 30, 2014, 6:05 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Patrick Reilly, and Vinod 
Kone.


Changes
---

Remove merged dependency.


Bugs: MESOS-1739
https://issues.apache.org/jira/browse/MESOS-1739


Repository: mesos-git


Description
---

Allows attributes and resources to be set to a superset of what they were 
previously on a slave restart.

Incorporates all comments from: 
https://issues.apache.org/jira/browse/MESOS-1739
and the former review request:
https://reviews.apache.org/r/25111/


Diffs (updated)
-

  src/Makefile.am 27c42dfde45a449750132e416b4eaf776f8c5e3b 
  src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 
  src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
  src/common/slaveinfo_utils.hpp PRE-CREATION 
  src/common/slaveinfo_utils.cpp PRE-CREATION 
  src/master/master.hpp d6380199421840aa17d4ce2725dcbcf4a11ce85f 
  src/master/master.cpp a60308f912a1ed81ecd51c677461a8f591d9eb8e 
  src/slave/slave.cpp e56dcbd80114730949a0d4b553470802a4d38281 
  src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b 
  src/tests/slave_tests.cpp 69be28f6e82b99e23424bd2be8294f715d8040d4 

Diff: https://reviews.apache.org/r/25525/diff/


Testing
---

make check on localhost


Thanks,

Cody Maloney

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-09-29 Thread Mesos ReviewBot


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


Bad patch!

Reviews applied: [25261]

Failed command: git apply --index 25261.patch

Error:
 error: patch failed: docs/mesos-c++-style-guide.md:82
error: docs/mesos-c++-style-guide.md: patch does not apply
error: patch failed: m4/ax_cxx_compile_stdcxx_11.m4:36
error: m4/ax_cxx_compile_stdcxx_11.m4: patch does not apply

- Mesos ReviewBot


On Sept. 29, 2014, 11:26 p.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25525/
> ---
> 
> (Updated Sept. 29, 2014, 11:26 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Patrick Reilly, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-1739
> https://issues.apache.org/jira/browse/MESOS-1739
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Allows attributes and resources to be set to a superset of what they were 
> previously on a slave restart.
> 
> Incorporates all comments from: 
> https://issues.apache.org/jira/browse/MESOS-1739
> and the former review request:
> https://reviews.apache.org/r/25111/
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 27c42dfde45a449750132e416b4eaf776f8c5e3b 
>   src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 
>   src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
>   src/common/slaveinfo_utils.hpp PRE-CREATION 
>   src/common/slaveinfo_utils.cpp PRE-CREATION 
>   src/master/master.hpp d6380199421840aa17d4ce2725dcbcf4a11ce85f 
>   src/master/master.cpp a60308f912a1ed81ecd51c677461a8f591d9eb8e 
>   src/slave/slave.cpp e56dcbd80114730949a0d4b553470802a4d38281 
>   src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b 
>   src/tests/slave_tests.cpp 69be28f6e82b99e23424bd2be8294f715d8040d4 
> 
> Diff: https://reviews.apache.org/r/25525/diff/
> 
> 
> Testing
> ---
> 
> make check on localhost
> 
> 
> Thanks,
> 
> Cody Maloney
> 
>

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-09-29 Thread Cody Maloney


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

(Updated Sept. 29, 2014, 11:26 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Patrick Reilly, and Vinod 
Kone.


Changes
---

Update for latest changes in master


Bugs: MESOS-1739
https://issues.apache.org/jira/browse/MESOS-1739


Repository: mesos-git


Description
---

Allows attributes and resources to be set to a superset of what they were 
previously on a slave restart.

Incorporates all comments from: 
https://issues.apache.org/jira/browse/MESOS-1739
and the former review request:
https://reviews.apache.org/r/25111/


Diffs (updated)
-

  src/Makefile.am 27c42dfde45a449750132e416b4eaf776f8c5e3b 
  src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 
  src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
  src/common/slaveinfo_utils.hpp PRE-CREATION 
  src/common/slaveinfo_utils.cpp PRE-CREATION 
  src/master/master.hpp d6380199421840aa17d4ce2725dcbcf4a11ce85f 
  src/master/master.cpp a60308f912a1ed81ecd51c677461a8f591d9eb8e 
  src/slave/slave.cpp e56dcbd80114730949a0d4b553470802a4d38281 
  src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b 
  src/tests/slave_tests.cpp 69be28f6e82b99e23424bd2be8294f715d8040d4 

Diff: https://reviews.apache.org/r/25525/diff/


Testing
---

make check on localhost


Thanks,

Cody Maloney

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-09-19 Thread Cody Maloney



> On Sept. 11, 2014, 9:53 p.m., Vinod Kone wrote:
> > OK. I went through parts of this review but I have a bigger suggestion in 
> > mind, before I get too much into the weeds.
> > 
> > I think it's worthwhile for you to write up a design doc similar to the 
> > framework info doc w.r.t. updating SlaveInfo. This will force you to think 
> > about the repercussions of changing each of the SlaveInfo fields on the 
> > Mesos stack (master/allocator/slave/tasks/executors). Are you game?
> 
> Cody Maloney wrote:
> I'll work on a design doc later today. Will let you know when I have it.
> 
> Vinod Kone wrote:
> are you still on track for the design doc?

Yes. I have a draft now which I'm getting a first sanity check on at the 
moment, hopefully will have that by Tuesday of next week. Once that is done 
I'll send it out to you, and then mesos-dev as a whole.


- Cody


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


On Sept. 13, 2014, 12:33 a.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25525/
> ---
> 
> (Updated Sept. 13, 2014, 12:33 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Patrick Reilly, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-1739
> https://issues.apache.org/jira/browse/MESOS-1739
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Allows attributes and resources to be set to a superset of what they were 
> previously on a slave restart.
> 
> Incorporates all comments from: 
> https://issues.apache.org/jira/browse/MESOS-1739
> and the former review request:
> https://reviews.apache.org/r/25111/
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9b973e5 
>   src/common/attributes.hpp 0a043d5 
>   src/common/attributes.cpp aab114e 
>   src/common/slaveinfo_utils.hpp PRE-CREATION 
>   src/common/slaveinfo_utils.cpp PRE-CREATION 
>   src/master/master.hpp b492600 
>   src/master/master.cpp d5db24e 
>   src/slave/slave.cpp 1b3dc73 
>   src/tests/attributes_tests.cpp 240a8ca 
>   src/tests/slave_tests.cpp 69be28f 
> 
> Diff: https://reviews.apache.org/r/25525/diff/
> 
> 
> Testing
> ---
> 
> make check on localhost
> 
> 
> Thanks,
> 
> Cody Maloney
> 
>

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-09-19 Thread Vinod Kone



> On Sept. 11, 2014, 9:53 p.m., Vinod Kone wrote:
> > OK. I went through parts of this review but I have a bigger suggestion in 
> > mind, before I get too much into the weeds.
> > 
> > I think it's worthwhile for you to write up a design doc similar to the 
> > framework info doc w.r.t. updating SlaveInfo. This will force you to think 
> > about the repercussions of changing each of the SlaveInfo fields on the 
> > Mesos stack (master/allocator/slave/tasks/executors). Are you game?
> 
> Cody Maloney wrote:
> I'll work on a design doc later today. Will let you know when I have it.

are you still on track for the design doc?


- Vinod


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


On Sept. 13, 2014, 12:33 a.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25525/
> ---
> 
> (Updated Sept. 13, 2014, 12:33 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Patrick Reilly, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-1739
> https://issues.apache.org/jira/browse/MESOS-1739
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Allows attributes and resources to be set to a superset of what they were 
> previously on a slave restart.
> 
> Incorporates all comments from: 
> https://issues.apache.org/jira/browse/MESOS-1739
> and the former review request:
> https://reviews.apache.org/r/25111/
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9b973e5 
>   src/common/attributes.hpp 0a043d5 
>   src/common/attributes.cpp aab114e 
>   src/common/slaveinfo_utils.hpp PRE-CREATION 
>   src/common/slaveinfo_utils.cpp PRE-CREATION 
>   src/master/master.hpp b492600 
>   src/master/master.cpp d5db24e 
>   src/slave/slave.cpp 1b3dc73 
>   src/tests/attributes_tests.cpp 240a8ca 
>   src/tests/slave_tests.cpp 69be28f 
> 
> Diff: https://reviews.apache.org/r/25525/diff/
> 
> 
> Testing
> ---
> 
> make check on localhost
> 
> 
> Thanks,
> 
> Cody Maloney
> 
>

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-09-13 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [25261, 25525]

All tests passed.

- Mesos ReviewBot


On Sept. 13, 2014, 12:33 a.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25525/
> ---
> 
> (Updated Sept. 13, 2014, 12:33 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Patrick Reilly, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-1739
> https://issues.apache.org/jira/browse/MESOS-1739
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Allows attributes and resources to be set to a superset of what they were 
> previously on a slave restart.
> 
> Incorporates all comments from: 
> https://issues.apache.org/jira/browse/MESOS-1739
> and the former review request:
> https://reviews.apache.org/r/25111/
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9b973e5 
>   src/common/attributes.hpp 0a043d5 
>   src/common/attributes.cpp aab114e 
>   src/common/slaveinfo_utils.hpp PRE-CREATION 
>   src/common/slaveinfo_utils.cpp PRE-CREATION 
>   src/master/master.hpp b492600 
>   src/master/master.cpp d5db24e 
>   src/slave/slave.cpp 1b3dc73 
>   src/tests/attributes_tests.cpp 240a8ca 
>   src/tests/slave_tests.cpp 69be28f 
> 
> Diff: https://reviews.apache.org/r/25525/diff/
> 
> 
> Testing
> ---
> 
> make check on localhost
> 
> 
> Thanks,
> 
> Cody Maloney
> 
>

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-09-12 Thread Cody Maloney


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

(Updated Sept. 13, 2014, 12:33 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, Patrick Reilly, and Vinod 
Kone.


Changes
---

Address Dominic's comments


Bugs: MESOS-1739
https://issues.apache.org/jira/browse/MESOS-1739


Repository: mesos-git


Description
---

Allows attributes and resources to be set to a superset of what they were 
previously on a slave restart.

Incorporates all comments from: 
https://issues.apache.org/jira/browse/MESOS-1739
and the former review request:
https://reviews.apache.org/r/25111/


Diffs (updated)
-

  src/Makefile.am 9b973e5 
  src/common/attributes.hpp 0a043d5 
  src/common/attributes.cpp aab114e 
  src/common/slaveinfo_utils.hpp PRE-CREATION 
  src/common/slaveinfo_utils.cpp PRE-CREATION 
  src/master/master.hpp b492600 
  src/master/master.cpp d5db24e 
  src/slave/slave.cpp 1b3dc73 
  src/tests/attributes_tests.cpp 240a8ca 
  src/tests/slave_tests.cpp 69be28f 

Diff: https://reviews.apache.org/r/25525/diff/


Testing
---

make check on localhost


Thanks,

Cody Maloney

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-09-12 Thread Cody Maloney



> On Sept. 12, 2014, 11:48 p.m., Dominic Hamon wrote:
> > src/common/slaveinfo_utils.cpp, line 44
> > 
> >
> > std::pair might be preferable.

T is a pointer to member function in this case. I could try specifying it more 
exactly as a type, but const T* isn't it. There were also some issues with type 
deduction in variadic templates if I recall correctly. Making it so it is 
vague/general 'T' rather than the pointer to member function specifically makes 
the code simpler to write.


> On Sept. 12, 2014, 11:48 p.m., Dominic Hamon wrote:
> > src/common/slaveinfo_utils.cpp, line 47
> > 
> >
> > the operator precedence here is a little tricky - can you simplify it a 
> > bit?
> > 
> > or maybe make it clear that T is actually a T* in the API.

There is only one operator there. It's calling a pointer to member function. It 
is one of those C++ things you really don't do often.


> On Sept. 12, 2014, 11:48 p.m., Dominic Hamon wrote:
> > src/common/slaveinfo_utils.cpp, line 64
> > 
> >
> > is there a reason you're taking copies instead of using const& 
> > throughout?

In this case we're constructing Attributes() around the protobuf object which 
forces a copy into the new class. Same goes for the resources ones. Inside the 
variadic template we're dealing with results of functions where value is what 
we want


> On Sept. 12, 2014, 11:48 p.m., Dominic Hamon wrote:
> > src/master/master.hpp, line 927
> > 
> >
> > nit: s/propogate/propagate/

fixed


> On Sept. 12, 2014, 11:48 p.m., Dominic Hamon wrote:
> > src/master/master.cpp, line 3032
> > 
> >
> > are these tabs?

They definitely aren't in the new version


> On Sept. 12, 2014, 11:48 p.m., Dominic Hamon wrote:
> > src/master/master.cpp, line 4222
> > 
> >
> > i wonder if it's worth splitting this method into two wrappers around a 
> > _readdSlave method to avoid the boolean parameter.

There is a restructuring in general of the recover logic which should happen at 
some point (I was avoiding it in this patchset to minimize jitter). The 
_reregisterSlaveExistingMaster for instance is very similar to 
_reregisterSlave, and readdSlave sometimes has the allocator in it and 
sometimes outside (It has three call sites now).


> On Sept. 12, 2014, 11:48 p.m., Dominic Hamon wrote:
> > src/tests/attributes_tests.cpp, line 70
> > 
> >
> > nit:
> > 
> > Attributes a = Attributes::parse(
> > "cpus:45.55; ports:[1-2, 3-5];");

fixed (although all the other tests in the file do it the other way)


> On Sept. 12, 2014, 11:48 p.m., Dominic Hamon wrote:
> > src/tests/attributes_tests.cpp, line 76
> > 
> >
> > EXPECT_FALSE(b.isSubsetOf(a)); etc?

added


> On Sept. 12, 2014, 11:48 p.m., Dominic Hamon wrote:
> > src/tests/slave_tests.cpp, line 995
> > 
> >
> > test for failure to recover with a subset?

There are a couple more test cases which should be added (make sure the new 
SlaveInfo is checkpointed, the changes persist after master failover)


> On Sept. 12, 2014, 11:48 p.m., Dominic Hamon wrote:
> > src/master/master.hpp, line 1198
> > 
> >
> > nit - keep the = on the previous line.

fixed


> On Sept. 12, 2014, 11:48 p.m., Dominic Hamon wrote:
> > src/Makefile.am, line 253
> > 
> >
> > please keep this in alphabetical order

fixed


- Cody


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


On Sept. 12, 2014, 11:33 p.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25525/
> ---
> 
> (Updated Sept. 12, 2014, 11:33 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Patrick Reilly, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-1739
> https://issues.apache.org/jira/browse/MESOS-1739
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Allows attributes and resources to be set to a superset of what they were

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-09-12 Thread Dominic Hamon


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



src/Makefile.am


please keep this in alphabetical order



src/common/attributes.hpp


i don't think these comments are necessary. the interface is very clear.



src/common/slaveinfo_utils.hpp


consider putting this in a namespace. at least mesos, probably 
mesos::slaveinfo_utils.



src/common/slaveinfo_utils.hpp


nit: s/new_/new/



src/common/slaveinfo_utils.cpp


these could be in an anonymous namespace too, which i think is preferable.



src/common/slaveinfo_utils.cpp


make sure it's clear this depends on the variadic template check in 
configure.



src/common/slaveinfo_utils.cpp


std::pair might be preferable.



src/common/slaveinfo_utils.cpp


the operator precedence here is a little tricky - can you simplify it a bit?

or maybe make it clear that T is actually a T* in the API.



src/common/slaveinfo_utils.cpp


is there a reason you're taking copies instead of using const& throughout?



src/master/master.hpp


nit: s/propogate/propagate/



src/master/master.hpp


nit - keep the = on the previous line.



src/master/master.cpp


are these tabs?



src/master/master.cpp


i wonder if it's worth splitting this method into two wrappers around a 
_readdSlave method to avoid the boolean parameter.



src/master/master.cpp


tab



src/tests/attributes_tests.cpp


nit:

Attributes a = Attributes::parse(
"cpus:45.55; ports:[1-2, 3-5];");



src/tests/attributes_tests.cpp


EXPECT_FALSE(b.isSubsetOf(a)); etc?



src/tests/slave_tests.cpp


test for failure to recover with a subset?


- Dominic Hamon


On Sept. 12, 2014, 4:33 p.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25525/
> ---
> 
> (Updated Sept. 12, 2014, 4:33 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Patrick Reilly, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-1739
> https://issues.apache.org/jira/browse/MESOS-1739
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Allows attributes and resources to be set to a superset of what they were 
> previously on a slave restart.
> 
> Incorporates all comments from: 
> https://issues.apache.org/jira/browse/MESOS-1739
> and the former review request:
> https://reviews.apache.org/r/25111/
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9b973e5 
>   src/common/attributes.hpp 0a043d5 
>   src/common/attributes.cpp aab114e 
>   src/common/slaveinfo_utils.hpp PRE-CREATION 
>   src/common/slaveinfo_utils.cpp PRE-CREATION 
>   src/master/master.hpp b492600 
>   src/master/master.cpp d5db24e 
>   src/slave/slave.cpp 1b3dc73 
>   src/tests/attributes_tests.cpp 240a8ca 
>   src/tests/slave_tests.cpp 69be28f 
> 
> Diff: https://reviews.apache.org/r/25525/diff/
> 
> 
> Testing
> ---
> 
> make check on localhost
> 
> 
> Thanks,
> 
> Cody Maloney
> 
>

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-09-12 Thread Cody Maloney



> On Sept. 11, 2014, 9:53 p.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 3122
> > 
> >
> > You should checkpoint the updated slave info!
> 
> Cody Maloney wrote:
> info contains the new slave info set at the command line. State contains 
> the old / from checkpoint info. This effectively used to set info to be what 
> was in the checkpoint file which didn't actually change anything because the 
> two were identical.

Added a line to checkpoint the info / persist it across reboots.


- Cody


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


On Sept. 12, 2014, 11:33 p.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25525/
> ---
> 
> (Updated Sept. 12, 2014, 11:33 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Patrick Reilly, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-1739
> https://issues.apache.org/jira/browse/MESOS-1739
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Allows attributes and resources to be set to a superset of what they were 
> previously on a slave restart.
> 
> Incorporates all comments from: 
> https://issues.apache.org/jira/browse/MESOS-1739
> and the former review request:
> https://reviews.apache.org/r/25111/
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9b973e5 
>   src/common/attributes.hpp 0a043d5 
>   src/common/attributes.cpp aab114e 
>   src/common/slaveinfo_utils.hpp PRE-CREATION 
>   src/common/slaveinfo_utils.cpp PRE-CREATION 
>   src/master/master.hpp b492600 
>   src/master/master.cpp d5db24e 
>   src/slave/slave.cpp 1b3dc73 
>   src/tests/attributes_tests.cpp 240a8ca 
>   src/tests/slave_tests.cpp 69be28f 
> 
> Diff: https://reviews.apache.org/r/25525/diff/
> 
> 
> Testing
> ---
> 
> make check on localhost
> 
> 
> Thanks,
> 
> Cody Maloney
> 
>

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-09-12 Thread Cody Maloney

> On Sept. 11, 2014, 9:53 p.m., Vinod Kone wrote:
> > OK. I went through parts of this review but I have a bigger suggestion in 
> > mind, before I get too much into the weeds.
> > 
> > I think it's worthwhile for you to write up a design doc similar to the 
> > framework info doc w.r.t. updating SlaveInfo. This will force you to think 
> > about the repercussions of changing each of the SlaveInfo fields on the 
> > Mesos stack (master/allocator/slave/tasks/executors). Are you game?

I'll work on a design doc later today. Will let you know when I have it.

> On Sept. 11, 2014, 9:53 p.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 3109
> > 
> >
> > Why is this function returning a Try instead of Try? I 
> > don't see how it is using the boolean?
> > 
> > Also,
> > 
> > s/res/compatible/

The bool is indicating whether or not anything changed. It is quite likely / 
possible that there won't be any change to the slaveInfo in which case we don't 
need to do anything special / fall into the old code path. This is relevant a 
lot more in the server than the slave. If the slave was allowing runtime 
updates it would be more critical there.

> On Sept. 11, 2014, 9:53 p.m., Vinod Kone wrote:
> > src/common/attributes.cpp, lines 161-172
> > 
> >
> > Why not just add "<=" and ">=" operator overloads like we did for 
> > Resources instead of adding subset/superset methods?
> > 
> > Also, please add tests for these in attributes_tests.cpp.

The style guide says we shouldn't overload those: 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Operator_Overloading.
 There isn't an exception in the mesos C++ style guide yet (Perhaps one should 
be added)?

For me it's not intuitive unless the type I'm working with that <= and >= do 
superset/subset. I had to read through the resource code to verify it was doing 
a subset/superset with <= (There is no >=, >, < on resources). Also note when 
giving these different behaviors, sometimes what people take to be true of 
operators (<= should return the exact opposite of >).

> On Sept. 11, 2014, 9:53 p.m., Vinod Kone wrote:
> > src/common/slaveinfo_utils.hpp, line 26
> > 
> >
> > I think this function name is misleading. It is not reconfiguring a 
> > slave, it is just doing a compatibility test.
> > 
> > Try isCompatible(
> >const mesos::SlaveInfo& newInfo,
> >const mesos::SlaveInfo& oldInfo);
> >
> > You should also add a comment on what this method is doing, when is it 
> > considered compatible etc.

Changed the function name. I don't want to write the compatibility guarantees 
in the header because somewhere down the line someone will come and update the 
function body but not see / change the header comment (And it would be hard to 
spot in a code review). The code is fairly readable for what is checked.

> On Sept. 11, 2014, 9:53 p.m., Vinod Kone wrote:
> > src/common/slaveinfo_utils.cpp, line 48
> > 
> >
> > I think we have to be careful about allowing 'checkpoint' to be changed.
> > 
> > If you haven't already, please refer to the design doc for updating 
> > FrameworkInfo 
> > (https://docs.google.com/document/d/1vEBuFN9mm3HkrNCmkAuwX-4kNYv0MwjUecLE3Jp_BqE/edit?usp=sharing)
> >  which talks about checkpointing.
> > 
> > In particular what are the semantics for already running 
> > tasks/executors if 'checkpoint' is changed?

Updated so that only attributes and resources can be changed. Everything else 
now must stay exactly the same.

> On Sept. 11, 2014, 9:53 p.m., Vinod Kone wrote:
> > src/common/slaveinfo_utils.cpp, line 66
> > 
> >
> > How about:
> > 
> > ```
> > Try isCompatible(const SlaveInfo& newSlaveInfo, const 
> > SlaveInfo& oldSlaveIfno)
> > {
> > 
> >   // ..
> >   if (!(newSlaveInfo.id() == oldSlaveInfo.id())) {
> > return Error(...);
> >   }
> > 
> >   //...
> >   if (newSlaveInfo.hostname() != oldSlaveInfo.hostname()) {
> > return Error(...);
> >   }
> > 
> >   //...
> >   if (newSlaveInfo.resources() < oldSlaveInfo.resources()) {
> > return Error(...);
> >   }
> >   
> >   //...
> >   if (newSlaveInfo.attributes() < oldSlaveInfo.attributes()) {
> > return Error(...);
> >   }
> > 
> >   return Nothing();
> > }
> > ```

It's important to note true / false for whether or not their are changes (It 
changes what code path we take in the server). In the case of an eventual API 
endpoint for updating slave info it will be critical to know and

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-09-12 Thread Cody Maloney


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

(Updated Sept. 12, 2014, 11:33 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Patrick Reilly, and Vinod 
Kone.


Changes
---

Update to address most of Vinod's comments


Bugs: MESOS-1739
https://issues.apache.org/jira/browse/MESOS-1739


Repository: mesos-git


Description
---

Allows attributes and resources to be set to a superset of what they were 
previously on a slave restart.

Incorporates all comments from: 
https://issues.apache.org/jira/browse/MESOS-1739
and the former review request:
https://reviews.apache.org/r/25111/


Diffs (updated)
-

  src/Makefile.am 9b973e5 
  src/common/attributes.hpp 0a043d5 
  src/common/attributes.cpp aab114e 
  src/common/slaveinfo_utils.hpp PRE-CREATION 
  src/common/slaveinfo_utils.cpp PRE-CREATION 
  src/master/master.hpp b492600 
  src/master/master.cpp d5db24e 
  src/slave/slave.cpp 1b3dc73 
  src/tests/attributes_tests.cpp 240a8ca 
  src/tests/slave_tests.cpp 69be28f 

Diff: https://reviews.apache.org/r/25525/diff/


Testing
---

make check on localhost


Thanks,

Cody Maloney

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-09-11 Thread Vinod Kone

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

OK. I went through parts of this review but I have a bigger suggestion in mind, 
before I get too much into the weeds.

I think it's worthwhile for you to write up a design doc similar to the 
framework info doc w.r.t. updating SlaveInfo. This will force you to think 
about the repercussions of changing each of the SlaveInfo fields on the Mesos 
stack (master/allocator/slave/tasks/executors). Are you game?

src/common/attributes.cpp

2 blank lines between outer elements.

src/common/attributes.cpp

Why not just add "<=" and ">=" operator overloads like we did for Resources 
instead of adding subset/superset methods?

Also, please add tests for these in attributes_tests.cpp.

src/common/attributes.cpp

2 blank lines between outer elements.

src/common/attributes.cpp

2 blank lines between outer elements.

src/common/slaveinfo_utils.hpp

I think this function name is misleading. It is not reconfiguring a slave, 
it is just doing a compatibility test.

Try isCompatible(
   const mesos::SlaveInfo& newInfo,
   const mesos::SlaveInfo& oldInfo);

You should also add a comment on what this method is doing, when is it 
considered compatible etc.

src/common/slaveinfo_utils.cpp

Typo? This sentence is incomplete.

src/common/slaveinfo_utils.cpp

Please use camelCase instead of snake case for variable names. There are 
only few exceptions in the code base to this rule, but they usually come with a 
good reason.

s/attributes_new/newAttributes/
s/resources_new/newResources/
s/resources_old/oldResources/

src/common/slaveinfo_utils.cpp

you can compare the slave ids directly, instead of strings, if you include 
common/type_utils.hpp

Error("SlaveID cannot be changed (old: " + stringify(old.id()) + ", new: " 
+ stringify(new.id()) + ")")

src/common/slaveinfo_utils.cpp

I think we have to be careful about allowing 'checkpoint' to be changed.

If you haven't already, please refer to the design doc for updating 
FrameworkInfo 
(https://docs.google.com/document/d/1vEBuFN9mm3HkrNCmkAuwX-4kNYv0MwjUecLE3Jp_BqE/edit?usp=sharing)
 which talks about checkpointing.

In particular what are the semantics for already running tasks/executors if 
'checkpoint' is changed?

src/common/slaveinfo_utils.cpp

also include the new and old attributes in the error message to ease 
debugging.

src/common/slaveinfo_utils.cpp

new line.

also, include the old and new resources in the error message.

src/common/slaveinfo_utils.cpp

How about:

```
Try isCompatible(const SlaveInfo& newSlaveInfo, const SlaveInfo& 
oldSlaveIfno)
{

  // ..
  if (!(newSlaveInfo.id() == oldSlaveInfo.id())) {
return Error(...);
  }

  //...
  if (newSlaveInfo.hostname() != oldSlaveInfo.hostname()) {
return Error(...);
  }

  //...
  if (newSlaveInfo.resources() < oldSlaveInfo.resources()) {
return Error(...);
  }

  //...
  if (newSlaveInfo.attributes() < oldSlaveInfo.attributes()) {
return Error(...);
  }

  return Nothing();
}
```

src/slave/slave.cpp

Why is this function returning a Try instead of Try? I don't 
see how it is using the boolean?

Also,

s/res/compatible/

src/slave/slave.cpp

I don't think there's much value left with logging the old and new slave 
info. The res.error() will pinpoint what the error is.

src/slave/slave.cpp

You should checkpoint the updated slave info!

- Vinod Kone

On Sept. 11, 2014, 8:13 a.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25525/
> ---
> 
> (Updated Sept. 11, 2014, 8:13 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hin

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-09-11 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [25525]

All tests passed.

- Mesos ReviewBot


On Sept. 11, 2014, 8:13 a.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25525/
> ---
> 
> (Updated Sept. 11, 2014, 8:13 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Patrick Reilly, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-1739
> https://issues.apache.org/jira/browse/MESOS-1739
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Allows attributes and resources to be set to a superset of what they were 
> previously on a slave restart.
> 
> Incorporates all comments from: 
> https://issues.apache.org/jira/browse/MESOS-1739
> and the former review request:
> https://reviews.apache.org/r/25111/
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9b973e5 
>   src/common/attributes.hpp 0a043d5 
>   src/common/attributes.cpp aab114e 
>   src/common/slaveinfo_utils.hpp PRE-CREATION 
>   src/common/slaveinfo_utils.cpp PRE-CREATION 
>   src/master/master.hpp b492600 
>   src/master/master.cpp d5db24e 
>   src/slave/slave.cpp 1b3dc73 
>   src/tests/slave_tests.cpp 69be28f 
> 
> Diff: https://reviews.apache.org/r/25525/diff/
> 
> 
> Testing
> ---
> 
> make check on localhost
> 
> 
> Thanks,
> 
> Cody Maloney
> 
>

Re: Review Request 25525: MESOS-1739: Allow slave reconfiguration on restart

2014-09-11 Thread Cody Maloney


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

(Updated Sept. 11, 2014, 8:13 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, Patrick Reilly, and Vinod 
Kone.


Changes
---

Add line in Makefile.am so that slaveinfo_utils.hpp is findable.


Bugs: MESOS-1739
https://issues.apache.org/jira/browse/MESOS-1739


Repository: mesos-git


Description
---

Allows attributes and resources to be set to a superset of what they were 
previously on a slave restart.

Incorporates all comments from: 
https://issues.apache.org/jira/browse/MESOS-1739
and the former review request:
https://reviews.apache.org/r/25111/


Diffs (updated)
-

  src/Makefile.am 9b973e5 
  src/common/attributes.hpp 0a043d5 
  src/common/attributes.cpp aab114e 
  src/common/slaveinfo_utils.hpp PRE-CREATION 
  src/common/slaveinfo_utils.cpp PRE-CREATION 
  src/master/master.hpp b492600 
  src/master/master.cpp d5db24e 
  src/slave/slave.cpp 1b3dc73 
  src/tests/slave_tests.cpp 69be28f 

Diff: https://reviews.apache.org/r/25525/diff/


Testing
---

make check on localhost


Thanks,

Cody Maloney

40 matches

Mail list logo