> On April 12, 2014, 2:54 a.m., Vinod Kone wrote: > > src/master/master.cpp, line 2678 > > <https://reviews.apache.org/r/18381/diff/7/?file=540066#file540066line2678> > > > > we should really consolidate 'deactivate' framework and 'disconnect' > > slave. They are essentially doing similar things to frameworks and slaves. > > maybe a TODO for now if you don't want to tackle that first. > > Adam B wrote: > Added a TODO in master.hpp. Would need significant refactoring to share > code across the Framework and Slave structs, deferring to a later review.
i just meant the consolidation of the verbs. we should either call it deactivate or disconnect for both frameworks and slaves. TODO is fine. > On April 12, 2014, 2:54 a.m., Vinod Kone wrote: > > src/slave/slave.cpp, line 371 > > <https://reviews.apache.org/r/18381/diff/7/?file=540073#file540073line371> > > > > s/error/message/ > > > > What happens if the slaves are upgraded before the master? > > Adam B wrote: > If the Slaves are upgraded before the master, then the master would only > send ShutdownMessages without the 'message' field. > What I hoped would happen: Since the 'message' field is optional, install > should pass None() to shutdown as an Option, and no error would be printed. > After investigating, I now realize that it will default to an empty > string, and I'll need to handle that instead of isNone(). Correct? > > I still think it would be nice if ProtobufProcess::install() could > convert an optional protobuf into an Option of that type. Yes. I think it will send it as an empty string. So just use string param instead of Option<string>. > On April 12, 2014, 2:54 a.m., Vinod Kone wrote: > > src/slave/slave.cpp, line 590 > > <https://reviews.apache.org/r/18381/diff/7/?file=540073#file540073line590> > > > > I know this is copy pasted from sched.cpp but can this be > > CHECK_NOTNULL(authenticatee)? > > Adam B wrote: > We actually want to make sure that authenticatee IS null before we create > a new Authenticatee. Looks like there's no CHECK_ISNULL(), so I'll leave this > as is. i'm clearly blind. CHECK is fine here. > On April 12, 2014, 2:54 a.m., Vinod Kone wrote: > > src/slave/slave.cpp, line 640 > > <https://reviews.apache.org/r/18381/diff/7/?file=540073#file540073line640> > > > > s/master.get()/self()/ > > Adam B wrote: > Slave::shutdown() exits without terminating if (from && master != from), > so I either need to pass in master or nothing. > I thought master was appropriate, since we actually got back a > future==false (authentication failure) response from the master, as opposed > to a slave-side error. > Would you rather I pass nothing? Or alter Slave::shutdown() to also allow > (from == self())? s/self()/UPID()/ this is how we manually call remote message handlers. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18381/#review40197 ----------------------------------------------------------- On April 3, 2014, 4:44 a.m., Adam B wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18381/ > ----------------------------------------------------------- > > (Updated April 3, 2014, 4:44 a.m.) > > > Review request for mesos and Vinod Kone. > > > Bugs: MESOS-804 > https://issues.apache.org/jira/browse/MESOS-804 > > > Repository: mesos-git > > > Description > ------- > > Added authentication support for slaves. > Fixes MESOS-804. > > Open Questions: > - Should AuthenticateMessage be replaced with AuthenticateFrameworkMessage, > or specify an Authenticatee type as coded here? > - When multiple entries for the same principal exist in the credentials file, > only the last entry is used. Acceptable behavior, but shouldn't this be > documented? > > > Diffs > ----- > > include/mesos/mesos.proto 37f8a7f > src/master/flags.hpp 024f86d > src/master/master.hpp b6b9983 > src/master/master.cpp 5d0ddb0 > src/messages/messages.proto bba17a9 > src/sasl/authenticatee.hpp 42a4eba > src/sasl/common.hpp PRE-CREATION > src/sched/sched.cpp 3684cfe > src/slave/flags.hpp d5c54c0 > src/slave/slave.hpp 15e23ce > src/slave/slave.cpp 6d901dc > src/tests/authentication_tests.cpp 127c5e6 > src/tests/cluster.hpp 11684d9 > src/tests/mesos.cpp ae3aeee > src/tests/sasl_tests.cpp 945426d > src/tests/slave_recovery_tests.cpp 72b6d42 > > Diff: https://reviews.apache.org/r/18381/diff/ > > > Testing > ------- > > make check; manually tested flatfile slave authentication success/failure. > Added new slave authentication unit tests in authentication_tests.cpp. > > > Thanks, > > Adam B > >
