Re: Review Request 54361: Added a test to ensure roles from multi-role framework are added.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54361/#review158161
---




src/tests/master_tests.cpp (line 4995)


This will invoke `removeFramework`, shall we make also add some logic to 
test the `removeFramework` here and deprecate patch 
https://reviews.apache.org/r/54363 ? As I saw that 
https://reviews.apache.org/r/54363 almost have same code as this test.

We can probably rename this as `AddAndRemoveFrameworkWithMultiRoles`?


- Guangya Liu


On 十二月 6, 2016, 1:58 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54361/
> ---
> 
> (Updated 十二月 6, 2016, 1:58 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6684
> https://issues.apache.org/jira/browse/MESOS-6684
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test to ensure roles from multi-role framework are added.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp dfedbbdf78e8054813872e9eeebccc7504097751 
> 
> Diff: https://reviews.apache.org/r/54361/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER="MasterTest.AddFrameworkWithMultipleRoles"
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 54361: Added a test to ensure roles from multi-role framework are added.

[Jay Guo]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54361/
---

(Updated Dec. 6, 2016, 1:58 p.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, and Qian Zhang.


Changes
---

change `testing done` section to reflect new test name.


Bugs: MESOS-6684
https://issues.apache.org/jira/browse/MESOS-6684


Repository: mesos


Description
---

Added a test to ensure roles from multi-role framework are added.


Diffs
-

  src/tests/master_tests.cpp dfedbbdf78e8054813872e9eeebccc7504097751 

Diff: https://reviews.apache.org/r/54361/diff/


Testing (updated)
---

make
make check GTEST_FILTER="MasterTest.AddFrameworkWithMultipleRoles"
make check


Thanks,

Jay Guo

Re: Review Request 54361: Added a test to ensure roles from multi-role framework are added.

[Jay Guo]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54361/
---

(Updated Dec. 6, 2016, 1:47 p.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, and Qian Zhang.


Changes
---

addressed guangya's comments


Bugs: MESOS-6684
https://issues.apache.org/jira/browse/MESOS-6684


Repository: mesos


Description
---

Added a test to ensure roles from multi-role framework are added.


Diffs (updated)
-

  src/tests/master_tests.cpp dfedbbdf78e8054813872e9eeebccc7504097751 

Diff: https://reviews.apache.org/r/54361/diff/


Testing
---

make
make check GTEST_FILTER="MasterTest.AcceptMultiRoleFramework"
make check


Thanks,

Jay Guo

Re: Review Request 54361: Added a test to ensure roles from multi-role framework are added.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54361/#review158143
---



The test case in `Testing Done` section should be udpated to 
`AddFrameworkWithMultipleRoles` as well.

- Guangya Liu


On 十二月 6, 2016, 3:05 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54361/
> ---
> 
> (Updated 十二月 6, 2016, 3:05 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6684
> https://issues.apache.org/jira/browse/MESOS-6684
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test to ensure roles from multi-role framework are added.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp dfedbbdf78e8054813872e9eeebccc7504097751 
> 
> Diff: https://reviews.apache.org/r/54361/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER="MasterTest.AcceptMultiRoleFramework"
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 54361: Added a test to ensure roles from multi-role framework are added.

[Guangya Liu]

> On 十二月 6, 2016, 12:52 a.m., Guangya Liu wrote:
> > src/tests/master_tests.cpp, line 4992
> > 
> >
> > EXPECT_SOME_EQ(expected.get(), parse);
> 
> Jay Guo wrote:
> We already did `ASSERT_SOME(parse)` when we get response, so I think 
> `EXPECT_EQ` is sufficient here.

This depend on you but we already have an example here 
https://github.com/apache/mesos/blob/master/src/tests/master_tests.cpp#L2024-L2033


> On 十二月 6, 2016, 12:52 a.m., Guangya Liu wrote:
> > src/tests/master_tests.cpp, line 4957
> > 
> >
> > When this role was added?
> 
> Jay Guo wrote:
> 
> https://github.com/apache/mesos/blob/master/src/master/http.cpp#L3254-L3268
> Default role `"*"` is always inserted in the response.

I was asking Joerg for why always add star role here, I think that for both 
multi-role and single role framework, there is no need to add star role as for 
multi-role framework, we should always display the roles that we specified for 
this framework and there is no default role for mult-role framework; for single 
role framework, the default role is star and also no need to handle the star 
role separately when getting roles via endpoint.


- Guangya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54361/#review158089
---


On 十二月 6, 2016, 3:05 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54361/
> ---
> 
> (Updated 十二月 6, 2016, 3:05 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6684
> https://issues.apache.org/jira/browse/MESOS-6684
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test to ensure roles from multi-role framework are added.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp dfedbbdf78e8054813872e9eeebccc7504097751 
> 
> Diff: https://reviews.apache.org/r/54361/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER="MasterTest.AcceptMultiRoleFramework"
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 54361: Added a test to ensure roles from multi-role framework are added.

[Jay Guo]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54361/
---

(Updated Dec. 6, 2016, 3:05 a.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, and Qian Zhang.


Changes
---

address guangya's comments


Bugs: MESOS-6684
https://issues.apache.org/jira/browse/MESOS-6684


Repository: mesos


Description
---

Added a test to ensure roles from multi-role framework are added.


Diffs (updated)
-

  src/tests/master_tests.cpp dfedbbdf78e8054813872e9eeebccc7504097751 

Diff: https://reviews.apache.org/r/54361/diff/


Testing
---

make
make check GTEST_FILTER="MasterTest.AcceptMultiRoleFramework"
make check


Thanks,

Jay Guo

Re: Review Request 54361: Added a test to ensure roles from multi-role framework are added.

[Jay Guo]

> On Dec. 6, 2016, 12:52 a.m., Guangya Liu wrote:
> > src/tests/master_tests.cpp, line 4957
> > 
> >
> > When this role was added?

https://github.com/apache/mesos/blob/master/src/master/http.cpp#L3254-L3268
Default role `"*"` is always inserted in the response.


> On Dec. 6, 2016, 12:52 a.m., Guangya Liu wrote:
> > src/tests/master_tests.cpp, line 4992
> > 
> >
> > EXPECT_SOME_EQ(expected.get(), parse);

We already did `ASSERT_SOME(parse)` when we get response, so I think 
`EXPECT_EQ` is sufficient here.


- Jay


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54361/#review158089
---


On Dec. 5, 2016, 6:13 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54361/
> ---
> 
> (Updated Dec. 5, 2016, 6:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6684
> https://issues.apache.org/jira/browse/MESOS-6684
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test to ensure roles from multi-role framework are added.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp dfedbbdf78e8054813872e9eeebccc7504097751 
> 
> Diff: https://reviews.apache.org/r/54361/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER="MasterTest.AcceptMultiRoleFramework"
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 54361: Added a test to ensure roles from multi-role framework are added.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54361/#review158089
---




src/tests/master_tests.cpp (line 4917)


How about s/AddMultiRoleFramework/AddFrameworkWithMultiRoles



src/tests/master_tests.cpp (line 4947)


s/response.get().body/response->body



src/tests/master_tests.cpp (line 4949)


ditto



src/tests/master_tests.cpp (line 4957)


When this role was added?



src/tests/master_tests.cpp (line 4967)


ditto



src/tests/master_tests.cpp (line 4978)


ditto



src/tests/master_tests.cpp (lines 4991 - 4992)


new line here



src/tests/master_tests.cpp (line 4992)


EXPECT_SOME_EQ(expected.get(), parse);


- Guangya Liu


On 十二月 5, 2016, 6:13 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54361/
> ---
> 
> (Updated 十二月 5, 2016, 6:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6684
> https://issues.apache.org/jira/browse/MESOS-6684
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test to ensure roles from multi-role framework are added.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp dfedbbdf78e8054813872e9eeebccc7504097751 
> 
> Diff: https://reviews.apache.org/r/54361/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER="MasterTest.AcceptMultiRoleFramework"
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 54361: Added a test to ensure roles from multi-role framework are added.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54361/#review157965
---



Patch looks great!

Reviews applied: [54359, 54360, 54361]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Dec. 5, 2016, 6:13 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54361/
> ---
> 
> (Updated Dec. 5, 2016, 6:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6684
> https://issues.apache.org/jira/browse/MESOS-6684
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test to ensure roles from multi-role framework are added.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp dfedbbdf78e8054813872e9eeebccc7504097751 
> 
> Diff: https://reviews.apache.org/r/54361/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER="MasterTest.AcceptMultiRoleFramework"
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 54361: Added a test to ensure roles from multi-role framework are added.

[Jay Guo]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54361/
---

(Updated Dec. 5, 2016, 6:13 a.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, and Qian Zhang.


Changes
---

rename test case


Bugs: MESOS-6684
https://issues.apache.org/jira/browse/MESOS-6684


Repository: mesos


Description
---

Added a test to ensure roles from multi-role framework are added.


Diffs (updated)
-

  src/tests/master_tests.cpp dfedbbdf78e8054813872e9eeebccc7504097751 

Diff: https://reviews.apache.org/r/54361/diff/


Testing
---

make
make check GTEST_FILTER="MasterTest.AcceptMultiRoleFramework"
make check


Thanks,

Jay Guo