----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14292/#review26747 -----------------------------------------------------------
src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp <https://reviews.apache.org/r/14292/#comment52063> Is there a plan for consolidating these two futures? Is there a specific version of mesos where we can merge them? src/java/src/org/apache/mesos/MesosSchedulerDriver.java <https://reviews.apache.org/r/14292/#comment52062> s/init/initializeWithCredentials/ ? Then maybe the comment isn't as necessary. src/master/master.hpp <https://reviews.apache.org/r/14292/#comment52065> It doesn't look like the master uses this? Could we just create the vector on the stack in initialize()? src/master/master.hpp <https://reviews.apache.org/r/14292/#comment52066> Leaving this here as a note from our discussion: Would it be cleaner to have a 'registering' map which stores the promises and then have a authenticated set which only contains authenticated pids? src/sched/sched.cpp <https://reviews.apache.org/r/14292/#comment52083> /enqueued/the dispatch to _authenticate() is enqueued/ src/sched/sched.cpp <https://reviews.apache.org/r/14292/#comment52081> Every argument you use in a bind or function object, and consequently in a dispatch or delay call, will be copied. So, I'm not sure this note is needed. Even if authenticationTimeout took a Future reference, it would be working on a copy of the future because the bind object copied it. src/sched/sched.cpp <https://reviews.apache.org/r/14292/#comment52085> It may be cleaner to merge the two blocks for the !ready and reauthenticate logic since they do the same thing. if (reauthenticate || !future.isReady()) { LOG(WARNING) << "Failed to authenticate with master " << master << ": " << (reauthenticate ? "Master changed" : (future.isFailed() ? future.failure() : "future discarded")); authenticating = None(); reauthenticate = false; // TODO(vinod): Add a limit on number of retries. dispatch(self(), &Self::authenticate); // Retry. return; } Then you can kill the block below :) And this will potentially avoid sending error("Master refused authentication") unnecessarily in the future. src/sched/sched.cpp <https://reviews.apache.org/r/14292/#comment52082> Even with: authenticationTimeout(Future<bool>& f) f would be a copy of the future when you called delay(), so I'm not sure this second note is needed? src/sched/sched.cpp <https://reviews.apache.org/r/14292/#comment52084> Can you log the timeout here? if (future.discard()) { LOG(WARNING) << "Timed out"; } src/sched/sched.cpp <https://reviews.apache.org/r/14292/#comment52069> The use of the word "Flag" tripped me up here a bit in that I first thought that reauthenticate was a command-line flag. - Ben Mahler On Oct. 7, 2013, 8:04 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14292/ > ----------------------------------------------------------- > > (Updated Oct. 7, 2013, 8:04 a.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, Joe Smith, Kevin > Sweeney, Tobias Weingartner, and Bill Farner. > > > Bugs: MESOS-704 > https://issues.apache.org/jira/browse/MESOS-704 > > > Repository: mesos-git > > > Description > ------- > > Added authentication support for scheduler driver and master. > > > Diffs > ----- > > include/mesos/mesos.proto 957576bbc1c73513a9591194d017f76fe562a616 > include/mesos/scheduler.hpp cf3ecdaaf40fd878a80fe0b6f7e61a0997329cbd > src/common/type_utils.hpp 674a8820c339c6446dfa7d444457477ab4512e79 > src/java/jni/construct.cpp b01bd7ae2eda2dc5e0dcd68848c65bd9f9ea81f0 > src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp > 6d2a03b6a88e71ac4e2e2d1ee8e15925e393ef3d > src/java/src/org/apache/mesos/MesosSchedulerDriver.java > 7ef1fe7755286bf92b94d7ece4f72d54e5b57a84 > src/master/flags.hpp d59e67d5b2799d6d7a37e9cfe7246ae7372091ac > src/master/master.hpp 0aeec7fc540d44c03c1171f31a7281a4b0055925 > src/master/master.cpp ce8365f082a5f96ef64e33e526cb5047dff52127 > src/python/native/mesos_scheduler_driver_impl.cpp > f25d41d38caf2701813dbec0d342a3b327e9dedf > src/sasl/authenticator.hpp 2f78cf0fdd97f0ddc3a6ebd162e6559497d708e4 > src/sched/sched.cpp c399f2481259683a8e178abb3478307042292f23 > > Diff: https://reviews.apache.org/r/14292/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
