Re: Review Request 63831: Fixed a bug that removed the suppressed framework from sorter.

2017-12-01 Thread Jiang Yan Xu 


> On Nov. 21, 2017, 10:27 a.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 430-437 (original), 431-438 (patched)
> > 
> >
> > Do we need a lambda here?
> 
> Jiang Yan Xu wrote:
> I kept the original use of lambdas. 
> 
> I think the benefit of labmdas here is more about readability: compared 
> to 
> 
> ```
> set removedRoles = oldRoles;
> foreach (const string& role, newRoles) {
>   result.erase(role);
> }
> ```
> 
> The constness and construction of the variable is clearly isolated into a 
> small block in a long method with many similar variables/code blocks.
> 
> However as pointed out by the TODO, we should probably just implement a 
> set difference operator so all these become one-liners. I'll do it later.
> 
> Alexander Rukletsov wrote:
> Sounds like a wrong tool for the job and hence confusing. I would have 
> used an explicit scope then. But maybe this is just me. I'll ask MPark for 
> the reasoning here. Since it has been here before, it's fine to leave it for 
> now. Regading introducing the set operator, FYI: 
> https://reviews.apache.org/r/57110/#comment239761

Oh ok, thanks for the pointer!


- Jiang Yan


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


On Nov. 27, 2017, 4:45 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63831/
> ---
> 
> (Updated Nov. 27, 2017, 4:45 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-8223
> https://issues.apache.org/jira/browse/MESOS-8223
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In `updateFramework`, we currently treat frameworks suppressing a role the 
> same way as frameworks moving off a role and this could lead to the framework 
> being removed from the framework sorter, which is unexpected. We should only 
> remove the framework from a framework sorter when it's moving away from the 
> corresponding role.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5ce9ceaa3a5f84a1e076d45448863c418531cc2b 
>   src/tests/scheduler_tests.cpp 45fc9c0cfccdb22c2e3e8d5de30c04575814a0e9 
> 
> 
> Diff: https://reviews.apache.org/r/63831/diff/2/
> 
> 
> Testing
> ---
> 
> make check.
> 
> Added a test.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 63831: Fixed a bug that removed the suppressed framework from sorter.

2017-12-01 Thread Alexander Rukletsov 

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


Ship it!




Ship It!

- Alexander Rukletsov


On Nov. 28, 2017, 12:45 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63831/
> ---
> 
> (Updated Nov. 28, 2017, 12:45 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-8223
> https://issues.apache.org/jira/browse/MESOS-8223
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In `updateFramework`, we currently treat frameworks suppressing a role the 
> same way as frameworks moving off a role and this could lead to the framework 
> being removed from the framework sorter, which is unexpected. We should only 
> remove the framework from a framework sorter when it's moving away from the 
> corresponding role.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5ce9ceaa3a5f84a1e076d45448863c418531cc2b 
>   src/tests/scheduler_tests.cpp 45fc9c0cfccdb22c2e3e8d5de30c04575814a0e9 
> 
> 
> Diff: https://reviews.apache.org/r/63831/diff/2/
> 
> 
> Testing
> ---
> 
> make check.
> 
> Added a test.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 63831: Fixed a bug that removed the suppressed framework from sorter.

2017-12-01 Thread Mesos Reviewbot Windows 

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



FAIL: Mesos libprocess-tests failed to build

Reviews applied: `['63741', '63830', '63831']`

Failed command: `cmake.exe --build . --target libprocess-tests --config Debug`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63831

Relevant logs:

- 
[libprocess-tests-build-cmake-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63831/logs/libprocess-tests-build-cmake-stdout.log):

```
  C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(62): warning 
C4996: 'GetVersionExW': was declared deprecated (compiling source file 
C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\state_machine_tests.cpp) 
[C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
  C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(451): warning 
C4996: 'GetVersionExW': was declared deprecated (compiling source file 
C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\state_machine_tests.cpp) 
[C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
  C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(805): warning 
C4244: 'argument': conversion from 'ULONG_PTR' to 'pid_t', possible loss of 
data (compiling source file 
C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\state_machine_tests.cpp) 
[C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
  C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(62): warning 
C4996: 'GetVersionExW': was declared deprecated (compiling source file 
C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\subprocess_tests.cpp) 
[C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
  C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(451): warning 
C4996: 'GetVersionExW': was declared deprecated (compiling source file 
C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\subprocess_tests.cpp) 
[C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
  C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(62): warning 
C4996: 'GetVersionExW': was declared deprecated (compiling source file 
C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\system_tests.cpp) 
[C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
  C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(451): warning 
C4996: 'GetVersionExW': was declared deprecated (compiling source file 
C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\system_tests.cpp) 
[C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
  C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(805): warning 
C4244: 'argument': conversion from 'ULONG_PTR' to 'pid_t', possible loss of 
data (compiling source file 
C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\subprocess_tests.cpp) 
[C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
  C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(805): warning 
C4244: 'argument': conversion from 'ULONG_PTR' to 'pid_t', possible loss of 
data (compiling source file 
C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\system_tests.cpp) 
[C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
  C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(62): warning 
C4996: 'GetVersionExW': was declared deprecated (compiling source file 
C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\time_tests.cpp) 
[C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
  C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(451): warning 
C4996: 'GetVersionExW': was declared deprecated (compiling source file 
C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\time_tests.cpp) 
[C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
  C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(805): warning 
C4244: 'argument': conversion from 'ULONG_PTR' to 'pid_t', possible loss of 
data (compiling source file 
C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\time_tests.cpp) 
[C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
  C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(62): warning 
C4996: 'GetVersionExW': was declared deprecated (compiling source file 
C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\ssl_tests.cpp) 
[C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
  C:\DCOS\mesos\mesos\3rdparty\stout\include\stout/windows/os.hpp(451): warning 
C4996: 'GetVersionExW': was declared deprecated (compiling source file 
C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\ssl_tests.cpp) 
[C:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]


"C:\DCOS\mesos\3rdparty\libprocess\src\tests\libp

Re: Review Request 63831: Fixed a bug that removed the suppressed framework from sorter.

2017-12-01 Thread Alexander Rukletsov 


> On Nov. 21, 2017, 6:27 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 430-437 (original), 431-438 (patched)
> > 
> >
> > Do we need a lambda here?
> 
> Jiang Yan Xu wrote:
> I kept the original use of lambdas. 
> 
> I think the benefit of labmdas here is more about readability: compared 
> to 
> 
> ```
> set removedRoles = oldRoles;
> foreach (const string& role, newRoles) {
>   result.erase(role);
> }
> ```
> 
> The constness and construction of the variable is clearly isolated into a 
> small block in a long method with many similar variables/code blocks.
> 
> However as pointed out by the TODO, we should probably just implement a 
> set difference operator so all these become one-liners. I'll do it later.

Sounds like a wrong tool for the job and hence confusing. I would have used an 
explicit scope then. But maybe this is just me. I'll ask MPark for the 
reasoning here. Since it has been here before, it's fine to leave it for now. 
Regading introducing the set operator, FYI: 
https://reviews.apache.org/r/57110/#comment239761


- Alexander


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


On Nov. 28, 2017, 12:45 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63831/
> ---
> 
> (Updated Nov. 28, 2017, 12:45 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-8223
> https://issues.apache.org/jira/browse/MESOS-8223
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In `updateFramework`, we currently treat frameworks suppressing a role the 
> same way as frameworks moving off a role and this could lead to the framework 
> being removed from the framework sorter, which is unexpected. We should only 
> remove the framework from a framework sorter when it's moving away from the 
> corresponding role.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5ce9ceaa3a5f84a1e076d45448863c418531cc2b 
>   src/tests/scheduler_tests.cpp 45fc9c0cfccdb22c2e3e8d5de30c04575814a0e9 
> 
> 
> Diff: https://reviews.apache.org/r/63831/diff/2/
> 
> 
> Testing
> ---
> 
> make check.
> 
> Added a test.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 63831: Fixed a bug that removed the suppressed framework from sorter.

2017-11-27 Thread Jiang Yan Xu 

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

(Updated Nov. 27, 2017, 4:45 p.m.)


Review request for mesos and Alexander Rukletsov.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

In `updateFramework`, we currently treat frameworks suppressing a role the same 
way as frameworks moving off a role and this could lead to the framework being 
removed from the framework sorter, which is unexpected. We should only remove 
the framework from a framework sorter when it's moving away from the 
corresponding role.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
5ce9ceaa3a5f84a1e076d45448863c418531cc2b 
  src/tests/scheduler_tests.cpp 45fc9c0cfccdb22c2e3e8d5de30c04575814a0e9 


Diff: https://reviews.apache.org/r/63831/diff/2/

Changes: https://reviews.apache.org/r/63831/diff/1-2/


Testing
---

make check.

Added a test.


Thanks,

Jiang Yan Xu

Re: Review Request 63831: Fixed a bug that removed the suppressed framework from sorter.

2017-11-27 Thread Jiang Yan Xu 


> On Nov. 21, 2017, 10:27 a.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 430-437 (original), 431-438 (patched)
> > 
> >
> > Do we need a lambda here?

I kept the original use of lambdas. 

I think the benefit of labmdas here is more about readability: compared to 

```
set removedRoles = oldRoles;
foreach (const string& role, newRoles) {
  result.erase(role);
}
```

The constness and construction of the variable is clearly isolated into a small 
block in a long method with many similar variables/code blocks.

However as pointed out by the TODO, we should probably just implement a set 
difference operator so all these become one-liners. I'll do it later.


> On Nov. 21, 2017, 10:27 a.m., Alexander Rukletsov wrote:
> > src/tests/scheduler_tests.cpp
> > Lines 1554-1556 (patched)
> > 
> >
> > How about something like this:
> > // This test verifies that if a framework decides to suppress its roles 
> > on reregisteration, no offers will be made.

The main difference between `NoOffersOnReregistrationWithAllRolesSuppressed` 
and `NoOffersWithAllRolesSuppressed` is the initial condition "a framework 
initially with no roles suppressing offers". Although your suggestion implies 
it, I was emphasizing it. How about:

```
This test verifies that if a framework (initially with no roles suppressed) 
decides to suppress offers for its roles on reregisteration, no offers will be 
made.
```

It's probably simpler to read than my original version.


- Jiang Yan


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


On Nov. 15, 2017, 11:05 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63831/
> ---
> 
> (Updated Nov. 15, 2017, 11:05 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-8223
> https://issues.apache.org/jira/browse/MESOS-8223
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In `updateFramework`, we currently treat frameworks suppressing a role the 
> same way as frameworks moving off a role and this could lead to the framework 
> being removed from the framework sorter, which is unexpected. We should only 
> remove the framework from a framework sorter when it's moving away from the 
> corresponding role.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f0fa7754e5968288edb15929efc9d244b177 
>   src/tests/scheduler_tests.cpp 45fc9c0cfccdb22c2e3e8d5de30c04575814a0e9 
> 
> 
> Diff: https://reviews.apache.org/r/63831/diff/1/
> 
> 
> Testing
> ---
> 
> make check.
> 
> Added a test.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 63831: Fixed a bug that removed the suppressed framework from sorter.

2017-11-21 Thread Alexander Rukletsov 

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




src/master/allocator/mesos/hierarchical.cpp
Lines 430-437 (original), 431-438 (patched)


Do we need a lambda here?



src/tests/scheduler_tests.cpp
Lines 1554-1556 (patched)


How about something like this:
// This test verifies that if a framework decides to suppress its roles on 
reregisteration, no offers will be made.


- Alexander Rukletsov


On Nov. 16, 2017, 7:05 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63831/
> ---
> 
> (Updated Nov. 16, 2017, 7:05 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-8223
> https://issues.apache.org/jira/browse/MESOS-8223
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In `updateFramework`, we currently treat frameworks suppressing a role the 
> same way as frameworks moving off a role and this could lead to the framework 
> being removed from the framework sorter, which is unexpected. We should only 
> remove the framework from a framework sorter when it's moving away from the 
> corresponding role.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f0fa7754e5968288edb15929efc9d244b177 
>   src/tests/scheduler_tests.cpp 45fc9c0cfccdb22c2e3e8d5de30c04575814a0e9 
> 
> 
> Diff: https://reviews.apache.org/r/63831/diff/1/
> 
> 
> Testing
> ---
> 
> make check.
> 
> Added a test.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 63831: Fixed a bug that removed the suppressed framework from sorter.

2017-11-16 Thread Mesos Reviewbot Windows 

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



PASS: Mesos patch 63831 was successfully built and tested.

Reviews applied: `['63741', '63830', '63831']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/63831

- Mesos Reviewbot Windows


On Nov. 16, 2017, 7:05 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63831/
> ---
> 
> (Updated Nov. 16, 2017, 7:05 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-8223
> https://issues.apache.org/jira/browse/MESOS-8223
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In `updateFramework`, we currently treat frameworks suppressing a role the 
> same way as frameworks moving off a role and this could lead to the framework 
> being removed from the framework sorter, which is unexpected. We should only 
> remove the framework from a framework sorter when it's moving away from the 
> corresponding role.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f0fa7754e5968288edb15929efc9d244b177 
>   src/tests/scheduler_tests.cpp 45fc9c0cfccdb22c2e3e8d5de30c04575814a0e9 
> 
> 
> Diff: https://reviews.apache.org/r/63831/diff/1/
> 
> 
> Testing
> ---
> 
> make check.
> 
> Added a test.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Review Request 63831: Fixed a bug that removed the suppressed framework from sorter.

2017-11-15 Thread Jiang Yan Xu 

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

Review request for mesos and Alexander Rukletsov.


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


Repository: mesos


Description
---

In `updateFramework`, we currently treat frameworks suppressing a role the same 
way as frameworks moving off a role and this could lead to the framework being 
removed from the framework sorter, which is unexpected. We should only remove 
the framework from a framework sorter when it's moving away from the 
corresponding role.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
f0fa7754e5968288edb15929efc9d244b177 
  src/tests/scheduler_tests.cpp 45fc9c0cfccdb22c2e3e8d5de30c04575814a0e9 


Diff: https://reviews.apache.org/r/63831/diff/1/


Testing
---

make check.

Added a test.


Thanks,

Jiang Yan Xu