----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26382/#review59070 -----------------------------------------------------------
looking good. some minor comments. are you going to follow up with a review for metrics based on source and reason? that would make this effort super useful. include/mesos/mesos.proto <https://reviews.apache.org/r/26382/#comment100345> Add a comment saying that this is unused. src/examples/java/TestFramework.java <https://reviews.apache.org/r/26382/#comment100362> "with reason" should come before "from source" to be consistent with what you did for other example frameworks above. src/master/master.cpp <https://reviews.apache.org/r/26382/#comment100366> This is not a duplicate of ResourceUsageChecker because this also accounts for executor's resources. See the note on top of ResourcesUsageChecker. src/master/master.cpp <https://reviews.apache.org/r/26382/#comment100367> s/REASON_INVALID_RESOURCES/REASON_TASK_INVALID/ because this is just like any other task validation error, just happens to be done here. src/master/master.cpp <https://reviews.apache.org/r/26382/#comment100386> why no reason? use REASON_TASK_PENDING src/master/master.cpp <https://reviews.apache.org/r/26382/#comment100373> I think it would be nice to have a reason for reconciliation related updates, e.g., "REASON_RECONCILIATION_REQUEST". Then we could've metrics for how many status updates are being generated due to reconciliation. src/master/master.cpp <https://reviews.apache.org/r/26382/#comment100374> Not sure if REASON_RECONCILIATON_REQUEST is more appropriate here than REASON_TASK_UNKNOWN. src/master/master.cpp <https://reviews.apache.org/r/26382/#comment100375> ditto. see above. src/scaling/nested_exec.py <https://reviews.apache.org/r/26382/#comment100384> why the change? the python style doesn't follow mesos's c++ style. src/scaling/nested_exec.py <https://reviews.apache.org/r/26382/#comment100385> ditto. src/slave/slave.cpp <https://reviews.apache.org/r/26382/#comment100377> ditto. src/slave/slave.cpp <https://reviews.apache.org/r/26382/#comment100378> why no reason? use REASON_TASK_PENDING src/slave/slave.cpp <https://reviews.apache.org/r/26382/#comment100380> just do this at #2229. if (pid != UPID()) { status.set_source(TaskStatus::SOURCE_EXECUTOR); } else { status.set_source(TaskStatus::SOURCE_SLAVE); } src/slave/slave.cpp <https://reviews.apache.org/r/26382/#comment100381> kill this, per above comment. src/slave/slave.cpp <https://reviews.apache.org/r/26382/#comment100383> this is already done inside Executor::terminateTask(). why the duplicate code? src/tests/common/http_tests.cpp <https://reviews.apache.org/r/26382/#comment100391> kill this. src/tests/master_authorization_tests.cpp <https://reviews.apache.org/r/26382/#comment100387> kill this. src/tests/reconciliation_tests.cpp <https://reviews.apache.org/r/26382/#comment100389> kill this. this is sent by the scheduler. - Vinod Kone On Oct. 29, 2014, 8:45 p.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26382/ > ----------------------------------------------------------- > > (Updated Oct. 29, 2014, 8:45 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 168a7a8c35ed1bf3f5bd6d7431b1e511bae7b789 > src/common/protobuf_utils.hpp 212d5124b9a4cc58e61719fa7f07a61cd166e834 > src/common/protobuf_utils.cpp a9b65e328c4c62bff7fbf5633dda25d742d79019 > src/examples/balloon_framework.cpp b05d5679fe2915142907af0b2dc00c6cd76eb9c1 > src/examples/java/TestFramework.java > bc593d0abfacb00690b1492b2b82c970f4e4de6d > 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/test_framework.cpp 187a611ebfe35cb13ee48aa5eca934cf55f34dea > src/master/master.cpp 762d2ff6c168ac212f70b43275692a77496a7fcd > src/scaling/nested_exec.py 17e61706c75e6e849b0c40ae5232d8dc60804c55 > src/sched/sched.cpp 0fb8c7bda75545389f8024489b3c76ae115111f4 > src/slave/slave.cpp 96fb5f7385b0762d46d8129f7e43207bd6311644 > src/tests/common/http_tests.cpp 912653b26615c86cc0204d80f58e6046c4b91a98 > src/tests/fault_tolerance_tests.cpp > a18a41a3e34ff112e04e693447d757403e5013bd > src/tests/master_authorization_tests.cpp > 652e80d0d4567b225c6ffb326725ddfde06f7fd3 > src/tests/master_tests.cpp 2e525749247626c05efb2f54a707599facb114b6 > src/tests/reconciliation_tests.cpp 4ba53940951584d9baa2c7aa4e124814471206bc > src/tests/resource_offers_tests.cpp > fe66432b1bf75ee25feb73c4bb353e4d4e5b503f > > Diff: https://reviews.apache.org/r/26382/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Dominic Hamon > >
