> 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? > > Ben Mahler wrote: > 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
interesting. are you planning to do a sweep of our code base and update per this new convention? otherwise, i prefer sticking to the former style to be consistent. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23542/#review48149 ----------------------------------------------------------- On July 21, 2014, 7:57 p.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23542/ > ----------------------------------------------------------- > > (Updated July 21, 2014, 7:57 p.m.) > > > Review request for mesos, Benjamin Hindman 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/master/master.hpp 7e7a75bd7e0fafc084ad2663c894e76e5fb35edd > src/master/master.cpp dc60c47b9c08b1e83fd72b6b86393fdc11314ea1 > 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 > >
