----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26382/#review58174 -----------------------------------------------------------
Some high level comments. --> Lets not introduce TASK_ERROR yet in status updates. I suspect most frameworks won't be ready to deal with it yet. Lets uncomment the state in TaskState but not use it yet. --> Email the dev list about the new TASK_ERROR state and these enums (pointing to the ticket) and let them know that TASK_ERROR will come in 0.22.0. Also tell them what the difference between TASK_ERROR and TASK_LOST is. --> Instead of letting executor setting SOURCE, have the slave set it. This way we can be sure that executors don't set the wrong source. Side benefit, this diff would be much smaller :) --> Lets kill FRAMEWORK from SOURCE. For TASK_LOST generated by driver, just set MASTER as SOURCE. We will get rid of those from the driver soon. include/mesos/mesos.proto <https://reviews.apache.org/r/26382/#comment99106> Pull this one and Reason inside TaskStatus. Also kill the SOURCE_ and REASON_ prefixes. include/mesos/mesos.proto <https://reviews.apache.org/r/26382/#comment99107> kill this. we'll use "MASTER" for the last remaining TASK_LOST case in the driver for now. include/mesos/mesos.proto <https://reviews.apache.org/r/26382/#comment99119> I think this will become a crutch. We should know the reasons for why a task is lost. i.e., kill this. include/mesos/mesos.proto <https://reviews.apache.org/r/26382/#comment99117> s/FAILED_VALIDATION/TASK_INVALID/ include/mesos/mesos.proto <https://reviews.apache.org/r/26382/#comment99130> s/NOT_ENOUGH_RESOURCES/INVALID_RESOURCES/ also, how is this different from FAILED_VALIDATION? include/mesos/mesos.proto <https://reviews.apache.org/r/26382/#comment99121> see my comment above. no need for default. src/common/http.cpp <https://reviews.apache.org/r/26382/#comment99123> what happens if this is not set? src/common/protobuf_utils.hpp <https://reviews.apache.org/r/26382/#comment99125> no default. src/examples/java/TestFramework.java <https://reviews.apache.org/r/26382/#comment99132> looks like you missed "from source" in other example frameworks above. src/examples/long_lived_executor.cpp <https://reviews.apache.org/r/26382/#comment99131> i would prefer for the slave to set it instead of the driver/executor for guaranteeing correctness. please fix it, here and everywhere else. src/master/master.cpp <https://reviews.apache.org/r/26382/#comment99133> why is this TASK_ERROR instead of TASK_LOST? A relaunch would likely succeed here right? src/slave/slave.cpp <https://reviews.apache.org/r/26382/#comment99245> not yours. but can you put each of the args on a different line? src/tests/allocator_tests.cpp <https://reviews.apache.org/r/26382/#comment99251> why is the source framework here instead of executor in all these tests? anyway, once you start setting the executor source in slave you wouldn't need to change the tests. - Vinod Kone On Oct. 21, 2014, 8:29 p.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26382/ > ----------------------------------------------------------- > > (Updated Oct. 21, 2014, 8:29 p.m.) > > > Review request for mesos, Vinod Kone and Bill Farner. > > > Bugs: MESOS-1830 and MESOS-343 > https://issues.apache.org/jira/browse/MESOS-1830 > https://issues.apache.org/jira/browse/MESOS-343 > > > Repository: mesos-git > > > Description > ------- > > Added source and reason, enabled TASK_ERROR, and made the changes necessary > throughout the codebase. > > > Diffs > ----- > > include/mesos/mesos.proto 6b93e9000761857c4f335f2a8c8088e155078f54 > src/common/http.cpp 58050e9534045ec3e8b74dd0214b1b12d5ff8ea7 > src/common/protobuf_utils.hpp 212d5124b9a4cc58e61719fa7f07a61cd166e834 > src/common/protobuf_utils.cpp a9b65e328c4c62bff7fbf5633dda25d742d79019 > src/examples/balloon_executor.cpp ad1d861aa6b5396f66e703326802926c7a99773c > src/examples/balloon_framework.cpp b05d5679fe2915142907af0b2dc00c6cd76eb9c1 > src/examples/java/TestExecutor.java > 3f385fbcbf4de3794be42bd1753d07632aba4dbe > src/examples/java/TestFramework.java > bc593d0abfacb00690b1492b2b82c970f4e4de6d > src/examples/long_lived_executor.cpp > d9b7fa1eefd1c53447244586797cb96a5c33d4f1 > src/examples/low_level_scheduler_libprocess.cpp > 7ef5ea78ade4ed856b97009fdfe31281f0a55c17 > src/examples/low_level_scheduler_pthread.cpp > 6e233a10117a1c7aa669806b5b430e746e227ee5 > src/examples/no_executor_framework.cpp > f98a0735b9f287e7f1bf98af6c2e9a47ca6a77b2 > src/examples/python/test_executor.py > f29da95b449bfb3aac655f02517f66403293ad10 > src/examples/test_executor.cpp 61c89d70e8eb5f855ca2490517766cc9e1a6065e > src/examples/test_framework.cpp 187a611ebfe35cb13ee48aa5eca934cf55f34dea > src/exec/exec.cpp e15f834beda94664b8e95c49ea87b4d36b853e71 > src/launcher/executor.cpp cbc8750a4d182251e6c15da6c7e75443d8c110bb > src/master/master.hpp 18898e9e1897b9d8cecd10d0ef7f25540cc9916d > src/master/master.cpp be910d9f02405e25e5ad6ae3bf13d770a0c2b417 > src/scaling/nested_exec.py 17e61706c75e6e849b0c40ae5232d8dc60804c55 > src/scaling/scaling_exec.py 04b4efcea22fe96af7854657ba6571a6814ee0e7 > src/sched/sched.cpp a37ed3d2e11035650b9bf0440fb87f66669129d8 > src/scheduler/scheduler.cpp fb88a3e029e97ba33eae5d50503be5ed9c9533e6 > src/slave/slave.hpp ccc0e03edbd41d9ce3bb0f6b0447bf4240648986 > src/slave/slave.cpp 7b5474ae898f369fc55670cf1dbbee684bc0ae4b > src/tests/allocator_tests.cpp 58e15aa3285b672a29edea4ea6e373273f28e339 > src/tests/common/http_tests.cpp 912653b26615c86cc0204d80f58e6046c4b91a98 > src/tests/fault_tolerance_tests.cpp > a75910d4f486230ba3f1d8927e5f1e5fda6e287b > src/tests/master_authorization_tests.cpp > 652e80d0d4567b225c6ffb326725ddfde06f7fd3 > src/tests/master_slave_reconciliation_tests.cpp > 04806ede4e727fa4de1464017a06797d69b54e29 > src/tests/master_tests.cpp d9dc40c6f5aaa66e1f7a0e1b7e4d9cdc586ca0fd > src/tests/mesos.hpp e40575c6d330cfa3fbfad2efcf601418c413b992 > src/tests/reconciliation_tests.cpp 400c5c035a2cadd408636021aafb3888546f0081 > src/tests/resource_offers_tests.cpp > 060039eed0bb8246ac034c0fb8560bfc7b25ab98 > src/tests/status_update_manager_tests.cpp > e9ef1e208cb01535e9366db7872b922c8f06ec40 > > Diff: https://reviews.apache.org/r/26382/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Dominic Hamon > >
