> On Feb. 18, 2014, 8:12 a.m., Niklas Nielsen wrote: > > src/master/master.cpp, line 2889 > > <https://reviews.apache.org/r/16724/diff/6/?file=492069#file492069line2889> > > > > Where does completedFramework clash (since the '_')?
I guess completedFramework doesn't actually clash here, but I thought it would be nice to iterate foreach completedFramework_ in completedFrameworks_ (clash), especially since we then pass it to readdCompletedFramework which has completedFramework_ as its parameter, so that readdCompletedFramework can iterate foreach completedFramework (clash) in completedFrameworks. I can remove the '_' from this completedFramework_ if you want, but I feel like it reads better this way. > On Feb. 18, 2014, 8:12 a.m., Niklas Nielsen wrote: > > src/messages/messages.proto, line 369 > > <https://reviews.apache.org/r/16724/diff/6/?file=492070#file492070line369> > > > > Is this comment obsolete? No, BenH wanted to actually move this into a separate file in a new mesos.internal.archive package, but that can be done when the rest of the Archive work comes along. > On Feb. 18, 2014, 8:12 a.m., Niklas Nielsen wrote: > > src/tests/mesos.hpp, lines 247-249 > > <https://reviews.apache.org/r/16724/diff/6/?file=492073#file492073line247> > > > > Any need to MergeFrom? If not, then use CopyFrom :) Not that I know of. Fixed the MergeFrom's in createTask as well. > On Feb. 18, 2014, 8:12 a.m., Niklas Nielsen wrote: > > src/tests/fault_tolerance_tests.cpp, line 666 > > <https://reviews.apache.org/r/16724/diff/6/?file=492072#file492072line666> > > > > s/Json/JSON/? I could find nothing in the Mesos/Google style guides about naming conventions with acronyms, so I went with a previous rule I've used. A quick google search yields the following recommendations: >From Microsoft: >http://msdn.microsoft.com/en-us/library/vstudio/ms229043(v=vs.100).aspx "Do capitalize only the first character of acronyms with three or more characters, except the first word of a camel-cased identifier." >From Geosoft (who?): http://geosoft.no/development/cppstyle.html "9. Abbreviations and acronyms must not be uppercase when used as name [4]." which actually cites Todd Hoff: http://www.possibility.com/Cpp/CppCodingStandard.html#noabbrev "Take for example NetworkABCKey. Notice how the C from ABC and K from key are confused. Some people don't mind this and others just hate it..." Looking in the Mesos code, I can find examples of both capitalization styles: 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp:13:TEST(JsonTest, BinaryData) 3rdparty/libprocess/src/process.cpp:2931: struct JSONVisitor : EventVisitor I can go either way, but I find it easier to read when the acronym doesn't bleed into the next capitalized letter. > On Feb. 18, 2014, 8:12 a.m., Niklas Nielsen wrote: > > src/tests/mesos.hpp, lines 239-252 > > <https://reviews.apache.org/r/16724/diff/6/?file=492073#file492073line239> > > > > You only use helper once, so how about moving it to > > fault_tolerance_tests.cpp (like findJsonValue)? Sure. Moved it, and renamed to createTaskWithDefaultExecutor (since it doesn't actually take an Executor parameter). If somebody else finds themselves needing it in another test, they can move it back to mesos.hpp. - Adam ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16724/#review34675 ----------------------------------------------------------- On Feb. 17, 2014, 4:23 p.m., Adam B wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16724/ > ----------------------------------------------------------- > > (Updated Feb. 17, 2014, 4:23 p.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, Niklas Nielsen, and > Vinod Kone. > > > Bugs: MESOS-767 > https://issues.apache.org/jira/browse/MESOS-767 > > > Repository: mesos-git > > > Description > ------- > > Added completed frameworks/tasks to slave re-registration. > Fixes MESOS-767. > > Additional issues discovered during investigation: > - MESOS-905: Remove Framework.id in favor of FrameworkInfo.id > - MESOS-906: Last task in Completed Framework never graduates from > terminatedTasks to completedTasks. > - Completed frameworks/executors/tasks are stored in circular buffers, > and these may overflow in different orders on different slaves. > BenH proposes an archive to replace these circular buffers. > > > Diffs > ----- > > include/mesos/scheduler.hpp 2e4707e > src/master/master.hpp 7649737 > src/master/master.cpp 77872ec > src/messages/messages.proto 922a8c4 > src/slave/slave.cpp 2d21e16 > src/tests/fault_tolerance_tests.cpp 60e06cc > src/tests/mesos.hpp d7bdaee > > Diff: https://reviews.apache.org/r/16724/diff/ > > > Testing > ------- > > make check; manually failed-over a master, watched the slave reregister its > completed frameworks, web UI shows completed tasks and stdout/stderr. > Added a new unit/integration test to verify the expected behavior. > > > Thanks, > > Adam B > >
