> On April 8, 2015, 5:52 p.m., Alexander Rukletsov wrote: > > src/master/master.cpp, lines 3480-3482 > > <https://reviews.apache.org/r/32502/diff/3/?file=915043#file915043line3480> > > > > Do you want to update metrics here? > > `++metrics->messages_reconcile_tasks;`
I'll add metrics in one sweep for all calls. > On April 8, 2015, 5:52 p.m., Alexander Rukletsov wrote: > > include/mesos/scheduler/scheduler.proto, line 191 > > <https://reviews.apache.org/r/32502/diff/3/?file=915041#file915041line191> > > > > I see your point in making `SlaveID` required in `TaskStatus` and it > > makes sense to me. But does this mean it should be required here as well? > > > > My understanding is that if the master knows `TaskID` it can always > > deduce `SlaveID`; if the task is unknown to the master, then the task is > > lost in all cases except when there are transitionary slaves. Having > > `SlaveID` in reconciliation query *just* speeds up reconciliation for > > unknown tasks, therefore making it required feels too much for me. Maybe > > Ben Mahler can provide more background and validate my thoughts. > > > > If you opt for making it required, I would then suggest to add checks > > in reconciliation algorithm, so that `SlaveID` of known (to the master) > > tasks matches `SlaveID` from the query. Similar to what you do in a > > subsequent patch for `killTask`. > > > > I'll also leave a comment on MESOS-1127. After some discussion with benm an benh, i've decided to make SlaveID optional in both reconcileTasks and killTask for being backwards compatible with frameworks that haven't persisted SlaveId with their tasks. > On April 8, 2015, 5:52 p.m., Alexander Rukletsov wrote: > > include/mesos/mesos.proto, line 768 > > <https://reviews.apache.org/r/32502/diff/3/?file=915040#file915040line768> > > > > For consistency, mind adding a comment in `Slave::statusUpdate()` that > > we should ensure `SlaveID` is set? Or even log warning in case it's not, so > > we start preparing framework authors for the upcoming change now. Also, > > maybe we need a JIRA for this. see below. i'll remove the TODO for now. > On April 8, 2015, 5:52 p.m., Alexander Rukletsov wrote: > > src/master/master.cpp, line 3476 > > <https://reviews.apache.org/r/32502/diff/3/?file=915043#file915043line3476> > > > > I would suggest we put the implementation after `Master::accept()` call > > for consistency. i'm keeping them close to their continuations for ease in readability in cpp. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32502/#review79358 ----------------------------------------------------------- On April 3, 2015, 11:36 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32502/ > ----------------------------------------------------------- > > (Updated April 3, 2015, 11:36 p.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-1127 > https://issues.apache.org/jira/browse/MESOS-1127 > > > Repository: mesos > > > Description > ------- > > Having a slave id will help us do better reconciliation. So added it to the > 'Reconcile' call. > > Also updated the master to receive the Reconcile call directly. > > > Diffs > ----- > > include/mesos/mesos.proto 3a8e8bf303e0576c212951f6028af77e54d93537 > include/mesos/scheduler/scheduler.proto > 783a63ad1cc0edd86605d638046fb959cb6e97e8 > src/master/master.hpp 05be911734b8d70c9fa5f3b4a275e8b610621fc5 > src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b > src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a > src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 > > Diff: https://reviews.apache.org/r/32502/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
