> On July 18, 2014, 6:38 p.m., Vinod Kone wrote: > > src/master/master.hpp, line 500 > > <https://reviews.apache.org/r/23542/diff/1/?file=633320#file633320line500> > > > > pull "||" up to the end of the previous lines?
I was thinking about the readability of conditional wrapping recently from a thread here, where the GNU formatting uses this style: https://news.ycombinator.com/item?id=7975386 https://www.gnu.org/prep/standards/html_node/Formatting.html Example to consider: if (recovered.contains(slaveId.get()) || reregistering.contains(slaveId.get()) || registered.contains(slaveId.get()) || removing.contains(slaveId.get()) { if (recovered.contains(slaveId.get()) || reregistering.contains(slaveId.get()) || registered.contains(slaveId.get()) || removing.contains(slaveId.get()) { The alignment of the || operator across lines exposes the underlying structure more clearly. Less "jaggedness" as we used to talk about. The google style guide refers to the former as being more common but allows both: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Boolean_Expressions#Boolean_Expressions > On July 18, 2014, 6:38 p.m., Vinod Kone wrote: > > src/master/master.cpp, line 3332 > > <https://reviews.apache.org/r/23542/diff/1/?file=633321#file633321line3332> > > > > I remember we talked about this. But sending a TASK_STAGING here might > > be better here for frameworks than ignoring? we could consider the act of > > validation/authorization and sending it to the slave all part of "staging" > > the task. Ok, since it's orthogonal to this change and needs to be done for the implicit reconciliation as well, I will follow up separately: https://issues.apache.org/jira/browse/MESOS-1620 > On July 18, 2014, 6:38 p.m., Vinod Kone wrote: > > src/tests/reconciliation_tests.cpp, lines 672-674 > > <https://reviews.apache.org/r/23542/diff/1/?file=633322#file633322line672> > > > > pull this below the offers expectation. It is below the offers expectation, something I'm missing? Is this only applicable to this test? - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23542/#review48149 ----------------------------------------------------------- On July 16, 2014, 3:16 a.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23542/ > ----------------------------------------------------------- > > (Updated July 16, 2014, 3:16 a.m.) > > > Review request for mesos, Niklas Nielsen and Vinod Kone. > > > Bugs: MESOS-1525 > https://issues.apache.org/jira/browse/MESOS-1525 > > > Repository: mesos-git > > > Description > ------- > > I would recommend applying this patch to review the reconcileTasks() logic > instead of using the diff viewer. > > Reconciliation requests currently specify a list of TaskStatuses. SlaveID is > optional inside TaskStatus but reconciliation requests are dropped when the > SlaveID is not specified. We can answer reconciliation requests for a task so > long as there are no transient slaves, this is what we should do when the > slave id is not specified. > > Also, I realized that we should answer when a non-strict registry is in use, > see the comment. > > > Diffs > ----- > > src/common/protobuf_utils.hpp 12ff00a2e1262d785dbd6ec4d54e50d88d611e20 > src/master/master.hpp 10bd95abc816c58fc2b72d3ce2d85128333cde39 > src/master/master.cpp 3ba8c334b38e16f072f59f13c718b2deb48a39f4 > src/tests/reconciliation_tests.cpp 6edbf7563375a69e3148e74a8cd99ddd13fc445b > > Diff: https://reviews.apache.org/r/23542/diff/ > > > Testing > ------- > > Removed a test that is now invalid and added a test for pending tasks. > > > Thanks, > > Ben Mahler > >
