----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22539/#review46574 -----------------------------------------------------------
Hey Zuyu, let's start with a review for the libprocess based low level scheduler example. I've included some comments in this review but please pull out the pthread example so we can get them reviewed and committed separately (and more quickly!). src/examples/low_level_scheduler_libprocess.cpp <https://reviews.apache.org/r/22539/#comment82027> Let's start in the DISCONNECTED state, is there a difference between DISCONNECTED and CONNECTING? src/examples/low_level_scheduler_libprocess.cpp <https://reviews.apache.org/r/22539/#comment82028> Let's move to a CONNECTED state in this method. That should control when we should be sending REGISTER messages. If we're disconnected, let's not send any REGISTER message to the master. src/examples/low_level_scheduler_libprocess.cpp <https://reviews.apache.org/r/22539/#comment82057> Do we need this? src/examples/low_level_scheduler_libprocess.cpp <https://reviews.apache.org/r/22539/#comment82064> What is an 'ev'? ;) s/ev/event/ src/examples/low_level_scheduler_libprocess.cpp <https://reviews.apache.org/r/22539/#comment82059> Let's consistently not use periods at the end of the log messages (some of your messages have periods and some don't). src/examples/low_level_scheduler_libprocess.cpp <https://reviews.apache.org/r/22539/#comment82060> "Slave " << ... << " failed"; src/examples/low_level_scheduler_libprocess.cpp <https://reviews.apache.org/r/22539/#comment82061> Please look at the executor id first as the slave id will be set when an executor failure is provided: Executor failure: slave_id and executor_id set Slave failure: slave_id set Also you can say: "Executor <id> failed on slave <id>" src/examples/low_level_scheduler_libprocess.cpp <https://reviews.apache.org/r/22539/#comment82062> This will only be set when executor id and slave id is set. src/examples/low_level_scheduler_libprocess.cpp <https://reviews.apache.org/r/22539/#comment82063> You should terminate if an error is encountered. src/examples/low_level_scheduler_libprocess.cpp <https://reviews.apache.org/r/22539/#comment82056> Let's remove this method and just use the << operator of the 'Resources' type from resources.hpp. Then we can easily print the resources inside 'recourceOffers'. src/examples/low_level_scheduler_libprocess.cpp <https://reviews.apache.org/r/22539/#comment82054> * on the type: Call::Launch* launch = ...; Here and everywhere else. src/examples/low_level_scheduler_libprocess.cpp <https://reviews.apache.org/r/22539/#comment82029> Let's just print similarly to how we print in test_framework.cpp so no need to print the message and data here. src/examples/low_level_scheduler_libprocess.cpp <https://reviews.apache.org/r/22539/#comment82026> s/doReliableConnecting/doReliableRegistration/ Let's also use "registered" in place of "connected" as the two are not the same (connected() callback does not mean registered). src/examples/low_level_scheduler_libprocess.cpp <https://reviews.apache.org/r/22539/#comment82030> Instead of having a done() function, can we just use finalize() (which is called when the process is terminated). That way, calls to 'done()' can be replaced with 'terminate(self())' and then we can send UNREGISTER from inside finalize(). src/examples/low_level_scheduler_libprocess.cpp <https://reviews.apache.org/r/22539/#comment82025> Let's rename these to match the actual states: DISCONNECTED = 0 CONNECTED = 1 REGISTERED = 2 src/examples/low_level_scheduler_libprocess.cpp <https://reviews.apache.org/r/22539/#comment82024> Doesn't look like we need the DONE state? - Ben Mahler On June 24, 2014, 3:44 p.m., Zuyu Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22539/ > ----------------------------------------------------------- > > (Updated June 24, 2014, 3:44 p.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Repository: mesos-git > > > Description > ------- > > Add the low level scheduler examples > > > Diffs > ----- > > src/Makefile.am 861aad264e89ae062d26b45f7dd3be1bc1a6d2cc > src/examples/low_level_scheduler.cpp PRE-CREATION > src/examples/low_level_scheduler_libprocess.cpp PRE-CREATION > src/tests/examples_tests.cpp 34f0233aca3433faba7528ac8c354100b8d3a4f7 > src/tests/low_level_scheduler_libprocess_test.sh PRE-CREATION > src/tests/low_level_scheduler_test.sh PRE-CREATION > > Diff: https://reviews.apache.org/r/22539/diff/ > > > Testing > ------- > > [==========] Running 2 tests from 1 test case. > [----------] Global test environment set-up. > [----------] 2 tests from ExamplesTest > [ RUN ] ExamplesTest.LowLevelScheduler > [ OK ] ExamplesTest.LowLevelScheduler (1647 ms) > [ RUN ] ExamplesTest.LowLevelSchedulerLibprocess > [ OK ] ExamplesTest.LowLevelSchedulerLibprocess (1698 ms) > [----------] 2 tests from ExamplesTest (3345 ms total) > > [----------] Global test environment tear-down > [==========] 2 tests from 1 test case ran. (3360 ms total) > [ PASSED ] 2 tests. > > > Thanks, > > Zuyu Zhang > >
