----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61189/#review183548 -----------------------------------------------------------
src/master/master.cpp Lines 9694 (patched) <https://reviews.apache.org/r/61189/#comment259564> Indent 4 more spaces. Here and below. src/master/master.cpp Lines 9766-9767 (patched) <https://reviews.apache.org/r/61189/#comment259559> Could you make the indentation here consistent with what's above? src/tests/api_tests.cpp Lines 2192 (patched) <https://reviews.apache.org/r/61189/#comment259565> s/Events/Event/ src/tests/api_tests.cpp Lines 2214 (patched) <https://reviews.apache.org/r/61189/#comment259570> Since you don't modify these flags at all, you can get rid of this. If you start the master without a flags parameter, it will use the flags produced by `CreateMasterFlags()` by default. src/tests/api_tests.cpp Lines 2222 (patched) <https://reviews.apache.org/r/61189/#comment259574> Can you move this down where you register the `connected` expectation on the scheduler? src/tests/api_tests.cpp Lines 2285 (patched) <https://reviews.apache.org/r/61189/#comment259575> You can get rid of this and then just do ``` call.mutable_subscribe()->mutable_framework_info()->CopyFrom(frameworkInfo); ``` src/tests/api_tests.cpp Lines 2334 (patched) <https://reviews.apache.org/r/61189/#comment259579> You can make this `ASSERT_EQ`, since the following line would likely crash hard if this event is of the wrong type. src/tests/api_tests.cpp Lines 2369 (patched) <https://reviews.apache.org/r/61189/#comment259583> Let's make these v1 TaskInfos instead of unversioned. src/tests/api_tests.cpp Lines 2422-2434 (patched) <https://reviews.apache.org/r/61189/#comment259587> Let's move this block directly below the `AWAIT_READY(update);` above on L2404. src/tests/api_tests.cpp Lines 2457 (patched) <https://reviews.apache.org/r/61189/#comment259589> s/execTask1/execTask2/ src/tests/api_tests.cpp Lines 2490 (patched) <https://reviews.apache.org/r/61189/#comment259591> I know we removed it previously, but let's add a line doing `EXPECT_TRUE(event.isPending());` after this. This will help ensure that there wasn't a heartbeat event waiting on the reader even before we launched task2. src/tests/api_tests.cpp Lines 2496-2497 (patched) <https://reviews.apache.org/r/61189/#comment259590> Could you add a comment here explaining what we're doing. Something like: "If the next event is a heartbeat, then we know that the TASK_ADDED event for `task2` was filtered correctly." - Greg Mann On Aug. 19, 2017, 3:33 a.m., Quinn Leng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61189/ > ----------------------------------------------------------- > > (Updated Aug. 19, 2017, 3:33 a.m.) > > > Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann. > > > Bugs: MESOS-7785 > https://issues.apache.org/jira/browse/MESOS-7785 > > > Repository: mesos > > > Description > ------- > > Added authorization filtering for the V1 operator events on the > master, the subscriber should only receive events that are > authorized based on their principal and ACLs. Since the authorizer > is called for every event emitted on the stream, the change of > authorization rule will affect events that the subscribers can > receive. > > > Diffs > ----- > > src/master/http.cpp b09a9d0ab5fab179d0e400932dce7d3ca958d7a8 > src/master/master.hpp d9cfc42646c874661e4ec79322126d93a2caf604 > src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99 > src/tests/api_tests.cpp 34480ea6818c904c64cb678562866f0b43dae0d6 > > > Diff: https://reviews.apache.org/r/61189/diff/5/ > > > Testing > ------- > > make check > GLOG_v=2 ./bin/mesos-tests.sh > --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" > --verbose --gtest_repeat=100 --gtest_break_on_failure > > > Thanks, > > Quinn Leng > >
