> On Nov. 28, 2017, 11:22 a.m., Jiang Yan Xu wrote: > > src/master/master.cpp > > Lines 6788 (patched) > > <https://reviews.apache.org/r/64098/diff/3/?file=1902267#file1902267line6788> > > > > I think this type of status updates could benefit from a distinct > > reason for to make it more strongly typed. > > > > Could you add > > > > ``` > > v1/mesos.proto: REASON_AGENT_REREGISTERED > > mesos.proto: REASON_SLAVE_REREGISTERED > > ``` > > > > and use it? > > > > Note that the tag numbers in the proto enum need to match. > > > > Also note that there are also documentation related changes for these > > fields: > > https://github.com/apache/mesos/blob/master/docs/task-state-reasons.md > > > > It's fine to make it a separate patch preceding this one. > > Ilya Pronin wrote: > Won't the new reason break any frameworks? > > Jiang Yan Xu wrote: > How will it break? We add new reasons all the time. This is indeed a new > sitaution that Mesos master sends status updates, how do you think the > framework will assert the reason otherwise? > > Ilya Pronin wrote: > Sure, the new reason reflects what's going on much better. I was thinking > that a framework that uses the old libmesos will get a "default" status > update reason, which is `REASON_COMMAND_EXECUTOR_FAILED`, and may act > incorrectly. But it seems that Aurora will be fine with such change. > > Megha Sharma wrote: > Yeah it seems that the frameworks should be fine with the addition of > another reason, I guess that's how we have been adding new reasons all along. > The proto default behavior to interpret the unknown enum value as first one > defined, is of course a problem but I believe the frameworks must already > have some guards in place to avoid that. > > Megha Sharma wrote: > +1 to add the new reasons: > > v1/mesos.proto: REASON_AGENT_REREGISTERED > mesos.proto: REASON_SLAVE_REREGISTERED
I see. Yeah the fact that there's not a sensible default value for the reason field is terrible. At least `REASON_COMMAND_EXECUTOR_FAILED` is deprecated: https://github.com/apache/mesos/blob/master/docs/task-state-reasons.md ``` The reason `REASON_COMMAND_EXECUTOR_FAILED` is deprecated and will be removed in the future. It should not be referenced by newly written code. ``` Hopefully we get rid of it soon and replace it with a `REASON_UNKONWN_REASON`. - Jiang Yan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64098/#review192041 ----------------------------------------------------------- On Nov. 27, 2017, 4:55 p.m., Megha Sharma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64098/ > ----------------------------------------------------------- > > (Updated Nov. 27, 2017, 4:55 p.m.) > > > Review request for mesos, Ilya Pronin, James Peach, and Jiang Yan Xu. > > > Bugs: MESOS-6406 > https://issues.apache.org/jira/browse/MESOS-6406 > > > Repository: mesos > > > Description > ------- > > Master will send task status updates to frameworks when an agent > re-registers. > > > Diffs > ----- > > src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f > src/tests/master_allocator_tests.cpp > 3400d70bb0ba564eac43c4639eee0efd4d8059e6 > src/tests/master_tests.cpp 9c450b9f592d9e09a468f537d9b500e97acc636b > src/tests/partition_tests.cpp e49c474167076b4136a161ed29b11db9a13455a7 > src/tests/persistent_volume_tests.cpp > acfeac16884b00581a3523607ff26f44f6dca53a > src/tests/slave_recovery_tests.cpp c864aa92d9ff128a89dbc25653385de25653f56a > src/tests/upgrade_tests.cpp 7f434dbba858f636719eec24e92b306b76430c4c > > > Diff: https://reviews.apache.org/r/64098/diff/4/ > > > Testing > ------- > > with make check > > > Thanks, > > Megha Sharma > >
