----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38874/#review103723 -----------------------------------------------------------
Ship it! Thanks for the patience and for reworking the patches! I think the CHECK_SOME in Executor::send is problematic, see below. I made some adjustments based on my comments in the review, please have a look over the commit diff! src/slave/slave.hpp (line 637) <https://reviews.apache.org/r/38874/#comment161793> Doesn't look like this should be a CHECK_SOME given the possibility of both being None, mind just logging a warning when we drop the message instead of the CHECK_SOME? src/slave/slave.hpp (line 685) <https://reviews.apache.org/r/38874/#comment161794> How about s/Unknown/Not known yet/ to suggest that we find out later? src/slave/slave.hpp <https://reviews.apache.org/r/38874/#comment161795> Hm.. doesn't this still need to be in the header? How do the output operations in Executor above compile? src/slave/slave.cpp (line 4114) <https://reviews.apache.org/r/38874/#comment161796> It looks like this call can trip the CHECK_SOME in Executor::send? :( src/slave/slave.cpp (line 5399) <https://reviews.apache.org/r/38874/#comment161797> The "at IP:PORT" should be sufficient, no need to say of type PID. src/slave/slave.cpp (line 5404) <https://reviews.apache.org/r/38874/#comment161799> how about "(via HTTP)" src/slave/slave.cpp (line 5406) <https://reviews.apache.org/r/38874/#comment161798> I think this will come out confusing in the logs, if I just read "of type: Unknown" it's not clear what the meaning of type is here. Would suggest saying nothing here. - Ben Mahler On Oct. 23, 2015, 1:34 a.m., Anand Mazumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38874/ > ----------------------------------------------------------- > > (Updated Oct. 23, 2015, 1:34 a.m.) > > > Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone. > > > Bugs: MESOS-3480 > https://issues.apache.org/jira/browse/MESOS-3480 > > > Repository: mesos > > > Description > ------- > > This change refactors the Executor struct on Agent and adds support for > Executors to connect via the `api/v1/executor` endpoint on Agent. This is > similar to the change done in Master for the Scheduler HTTP API. > > > Diffs > ----- > > src/slave/slave.hpp d81a9c4d09d3f6be417120e412324258f19604a3 > src/slave/slave.cpp e9f2d1bf7978450f0cd471bdb5606305092fc98a > > Diff: https://reviews.apache.org/r/38874/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Anand Mazumdar > >
