----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57743/#review170063 -----------------------------------------------------------
Getting there! Mostly minor comments around style/logging + a possible race condition that needs to be handled. src/slave/slave.cpp Lines 118 (patched) <https://reviews.apache.org/r/57743/#comment242832> Nit: You only seem to use it once. Can you directly invoke it with `common::validation::validateSecret` at the call-site? src/slave/slave.cpp Lines 2557 (patched) <https://reviews.apache.org/r/57743/#comment242834> Nit: Use the `->` operator instead. src/slave/slave.cpp Lines 2557 (patched) <https://reviews.apache.org/r/57743/#comment242854> Nit: Use the `->` operator instead of `get()` src/slave/slave.cpp Lines 2558 (patched) <https://reviews.apache.org/r/57743/#comment242855> Any reasons why you did not use the `!=` operator directly? Also, can you log the type of secret that was produced by the generator? src/slave/slave.cpp Lines 2599-2613 (patched) <https://reviews.apache.org/r/57743/#comment242837> Can we combine these? Let's have a temporary `executorState` to capture the `terminating`/`terminated` string. src/slave/slave.cpp Lines 2601 (patched) <https://reviews.apache.org/r/57743/#comment242840> s/with/in s/ID// Ditto for the other occurences. Also, you can directly use the `Executor` output operator. ```cpp LOG(WARNING) << "Ignoring launching executor " << *executor << " in container " << executor->containerId << " because the executor is " << executorState; ``` src/slave/slave.cpp Lines 2604 (patched) <https://reviews.apache.org/r/57743/#comment242846> hmm, this is problematic: You need to call `executorTerminated(...)` for both these cases too. If the framework is explicitly shutdown by the scheduler before the secret could be generated, we still need clean up the executor. src/slave/slave.cpp Lines 2614 (patched) <https://reviews.apache.org/r/57743/#comment242856> Explicitly assert that the executor state is `REGISTERING` here? src/slave/slave.cpp Lines 2619-2622 (patched) <https://reviews.apache.org/r/57743/#comment242843> Modify this as per earlier comment to use the output operator. src/slave/slave.cpp Lines 2622 (patched) <https://reviews.apache.org/r/57743/#comment242844> Can you also log the `Future` failure reason? src/slave/slave.cpp Lines 6606-6608 (original) <https://reviews.apache.org/r/57743/#comment242850> Can we keep this `TODO` for now? Looks like we can now revisit this given that `launchExecutor` is async. - Anand Mazumdar On March 24, 2017, 9:20 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57743/ > ----------------------------------------------------------- > > (Updated March 24, 2017, 9:20 p.m.) > > > Review request for mesos, Anand Mazumdar, Jie Yu, and Vinod Kone. > > > Bugs: MESOS-6999 > https://issues.apache.org/jira/browse/MESOS-6999 > > > Repository: mesos > > > Description > ------- > > This patch updates the agent code to generate executor > authentication tokens when executor authentication is > enabled. For now, the generated `Secret` objects must > be of `VALUE` type, and they're passed directly into the > executor environment. > > > Diffs > ----- > > src/slave/slave.hpp e2de66cc5b899b8b9a9ea27cc30f19a9e8fc11fb > src/slave/slave.cpp a4f4a9ca80b726de8e07571fd6d93120947c278b > > > Diff: https://reviews.apache.org/r/57743/diff/10/ > > > Testing > ------- > > Testing details can be found at the end of this chain. > > > Thanks, > > Greg Mann > >
