> On Feb. 7, 2015, 5:40 p.m., Benjamin Hindman wrote: > > src/master/contender.cpp, line 88 > > <https://reviews.apache.org/r/30195/diff/6/?file=853137#file853137line88> > > > > How can this ever be the case? What path brings us to this that doesn't > > go through a flags load? Can you document as much for posterity?
Added documentation string. I don't think MasterContender is part of the libmesos public api currently, although I don't think anyone actually uses it. That one is mainly for safety. The file:// url passed directly to MasterDetector is externally reachable + used by some linked frameworks. Outlined my plan for eliminating the need to do this entirely in the new comment. > On Feb. 7, 2015, 5:40 p.m., Benjamin Hindman wrote: > > src/master/contender.cpp, lines 90-92 > > <https://reviews.apache.org/r/30195/diff/6/?file=853137#file853137line90> > > > > Why deprecate this? Are you just trying to force people to use flags? > > What are the paths where that doesn't work? I want to push people towards using flags, and frameworks which will likely introspect the --master argument (Chronos I know does), are aware that there is a level of indirection this might "hide" from them. It is a much cleaner end solution I think to move the libmesos API so that if people want "mesos standard" handling of options they get it from mesos, and otherwise they can provide the "final" version of all parameters, handling processing of file:// and the like themselves. > On Feb. 7, 2015, 5:40 p.m., Benjamin Hindman wrote: > > src/master/detector.cpp, line 213 > > <https://reviews.apache.org/r/30195/diff/6/?file=853139#file853139line213> > > > > See comment above. Added a documentation string here as well. > On Feb. 7, 2015, 5:40 p.m., Benjamin Hindman wrote: > > src/tests/master_allocator_tests.cpp, line 1101 > > <https://reviews.apache.org/r/30195/diff/6/?file=853146#file853146line1101> > > > > Why remove the 'file://' here? I did this as part of generic cleanup path. Since masterFlags.whitelist is a path, it doesn't need to have the 'file://' prefix. We can leave it, but it feels odd to me to explicitly add the file:// path here, then have it stripped off immediately when we construct a Path() from the string, then do the swap for the assignment operator. - Cody ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30195/#review71559 ----------------------------------------------------------- On Feb. 9, 2015, 12:43 a.m., Cody Maloney wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30195/ > ----------------------------------------------------------- > > (Updated Feb. 9, 2015, 12:43 a.m.) > > > Review request for mesos, Benjamin Hindman and Ken Sipe. > > > Bugs: mesos-1806 > https://issues.apache.org/jira/browse/mesos-1806 > > > Repository: mesos > > > Description > ------- > > Mostly simplifies things. There are two places where there things get > complicated > > 1. --whitelist: This code wants a file to watch for changes and update > the whitelist from periodically. Introduces Path to work around this. > 2. --credential, --credentials: Custom parsing logic since the > credentials can be given in two different formats. Once the old > format is removed, either teaching <stout/flags> how to parse json > to protobuf generally or taking the flag as JSON then converting it > to protobuf later will make this clean. > > > Diffs > ----- > > CHANGELOG f515bc937441a2b4cc7e33bb857cb48a21aedea0 > docs/configuration.md 22f9e3db7b0e1691018de5fe3dfea3cb908de4b9 > docs/upgrades.md 51c7e70c7ddcd3d5f678872553a18eb27622f052 > src/credentials/credentials.hpp 9965858cedceecf29517f8a9b9430f3535164eb0 > src/examples/load_generator_framework.cpp > f803d9258b45fd406fcd57ee215418a9e932eb27 > src/master/contender.hpp 8e3e25aba8e93e13abf0815c9c547928f7ac2d7d > src/master/contender.cpp 0a8c099e51ffb4f17c6d635472799c33441e943c > src/master/detector.hpp 48107483150d90e3ebbc83ca4fac5cc872704ff1 > src/master/detector.cpp 367d1e1c76674c2376060ee18fe32fac2e091dc6 > src/master/flags.hpp df2e9cbdd56e9831290bf57c65b212cd7820a7f6 > src/master/main.cpp d4adae5a6044aef9f7cc214f0f467359b6f7a29a > src/master/master.cpp 7c3aa220e72fdc156fb9a0998dd68beb7c464256 > src/slave/flags.hpp f6033355d129f0013d39dd053455c936596bf159 > src/slave/slave.cpp fff2d725fe49eee984d9151cfb2131202c47994f > src/tests/credentials_tests.cpp e39db9e25d56d5688ac680a5bd0d1c525241999d > src/tests/master_allocator_tests.cpp > 1eebefd2e423e4bb89d76ed7b7d8acc9d1bb7760 > src/tests/master_tests.cpp b52d2caa55d28d00e036f7e2142952f357a07aa3 > src/tests/mesos.cpp 21a405366f56c963611324076efe775f85b9d9f7 > src/watcher/whitelist_watcher.hpp 16ea839b364196b1b0f3997d915be8b49804876c > src/watcher/whitelist_watcher.cpp 2a1586e92e1765edba28b89240d4eb44ce67840f > src/zookeeper/url.hpp 16e711c5c0bc29b1967a20f0827238f8a7b0deaf > > Diff: https://reviews.apache.org/r/30195/diff/ > > > Testing > ------- > > make distcheck > - Tests not hitting the file:// handling which were replaced with CHECK() > statements. > > gcc 4.4 + gcc 4.8 > > > Thanks, > > Cody Maloney > >
