Re: Review Request 63943: Renamed `TaskStatusEq()` to `TaskStatusTaskIdEq()`.

2017-11-19 Thread Mesos Reviewbot Windows 

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



FAIL: mesos-java failed to build.

Reviews applied: `['63577', '63943']`

Failed command: `cmake.exe --build . --target mesos-java`

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

Relevant logs:

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

```
  
C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3500):
 warning C4996: 'inet_ntoa': Use inet_ntop() or InetNtop() instead or define 
_WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3501):
 warning C4996: 'sprintf': This function or variable may be unsafe. Consider 
using sprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
See online help for details. 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3479):
 warning C4101: 'addrstr': unreferenced local variable 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(256):
 warning C4090: 'function': different 'const' qualifiers 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\mt_adaptor.c(496):
 warning C4244: '=': conversion from 'time_t' to 'int32_t', possible loss of 
data 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
c:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(166):
 warning C4716: 'pthread_cond_broadcast': must return a value 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
c:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(205):
 warning C4716: 'pthread_cond_wait': must return a value 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(124):
 warning C4996: 'fopen': This function or variable may be unsafe. Consider 
using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See 
online help for details. 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(128):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(279):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(301):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(368):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(372):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(512):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data

Re: Review Request 61157: Refactored ProcessManager::handle for future use with http::Server.

2017-11-19 Thread Mesos Reviewbot Windows 

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



FAIL: mesos-java failed to build.

Reviews applied: `['63941', '63942', '61155', '61156', '61157']`

Failed command: `cmake.exe --build . --target mesos-java`

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

Relevant logs:

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

```
  
C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3500):
 warning C4996: 'inet_ntoa': Use inet_ntop() or InetNtop() instead or define 
_WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3501):
 warning C4996: 'sprintf': This function or variable may be unsafe. Consider 
using sprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
See online help for details. 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3479):
 warning C4101: 'addrstr': unreferenced local variable 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(256):
 warning C4090: 'function': different 'const' qualifiers 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\mt_adaptor.c(496):
 warning C4244: '=': conversion from 'time_t' to 'int32_t', possible loss of 
data 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
c:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(166):
 warning C4716: 'pthread_cond_broadcast': must return a value 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
c:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(205):
 warning C4716: 'pthread_cond_wait': must return a value 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(124):
 warning C4996: 'fopen': This function or variable may be unsafe. Consider 
using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See 
online help for details. 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(128):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(279):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(301):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(368):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(372):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[C:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
  
C:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(512):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data

Re: Review Request 63912: Changed agent (re-)registration to set resource versions.

2017-11-19 Thread Jie Yu 

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


Fix it, then Ship it!





src/master/master.cpp
Lines 6257-6258 (patched)


Can we make this part of the `Slave`'s constructor (and no default value so 
that the caller will be forced to set it).



src/master/master.cpp
Lines 6847-6848 (patched)


Ditto.


- Jie Yu


On Nov. 17, 2017, 1:07 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63912/
> ---
> 
> (Updated Nov. 17, 2017, 1:07 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> An agent will send its resource version UUIDs during (re-)registration.
> The master will have to use them when sending a
> 'ApplyOfferOperationMessage' to an agent.
> This was missing in the master, the resource version were only set when
> an agent sent a 'UpdateSlaveMessage'. This created situations where the
> master would crash.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp a309fc78ee2613762f3d5d22ac7559afc7aac4a3 
>   src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
> 
> 
> Diff: https://reviews.apache.org/r/63912/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 61155: Added http::Server.

2017-11-19 Thread Benjamin Mahler 

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


Fix it, then Ship it!





3rdparty/libprocess/src/http.cpp
Lines 2096 (patched)


The run loop will erase the clients as they transition, was this required?



3rdparty/libprocess/src/http.cpp
Lines 2098-2100 (patched)






3rdparty/libprocess/src/tests/http_tests.cpp
Lines 2337 (patched)


Maybe a little comment about what this tests? E.g. ensures that the server 
does not re-order responses if handlers complete the responses out of order?



3rdparty/libprocess/src/tests/http_tests.cpp
Lines 2355-2360 (patched)


Hopefully this will become synchronous with your update?



3rdparty/libprocess/src/tests/http_tests.cpp
Lines 2419-2423 (patched)


Hm.. this doesn't make sure that 2 arrives before 3, wonder if there is an 
easy way to check that?



3rdparty/libprocess/src/tests/http_tests.cpp
Lines 2436 (patched)


StopNotRunning?


- Benjamin Mahler


On July 27, 2017, 1:55 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61155/
> ---
> 
> (Updated July 27, 2017, 1:55 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added http::Server.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> ba1b086a1dba140df491387077723d5c359acbcc 
>   3rdparty/libprocess/src/http.cpp fa3dde68de2e543ad910febbc4c8c46309b9c147 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> ef84967a034b6293e0279b61607c07ccc2526d99 
> 
> 
> Diff: https://reviews.apache.org/r/61155/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 63942: Added a state machine abstraction.

2017-11-19 Thread Benjamin Mahler 

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


Fix it, then Ship it!





3rdparty/libprocess/include/process/state_machine.hpp
Lines 166-181 (patched)


It seems a little odd to template these functions instead of pass the state 
as arguments, ditto elsewhere.



3rdparty/libprocess/src/tests/state_machine_tests.cpp
Lines 21 (patched)


Include for Try?



3rdparty/libprocess/src/tests/state_machine_tests.cpp
Lines 51 (patched)


This looks broken? Should result in a `Try`?



3rdparty/libprocess/src/tests/state_machine_tests.cpp
Lines 93 (patched)


Do we want to test when the future is notified relative to the completion 
of the lambda?


- Benjamin Mahler


On Nov. 20, 2017, 1:02 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63942/
> ---
> 
> (Updated Nov. 20, 2017, 1:02 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> An actor is often implemented like a state machine but the actual
> management of the state and transitions is very manual. This
> abstraction helps reduce a lot of the boilerplate assocaited with
> state machines.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 03a0ca87f31744c716c99e05aa07242fed480675 
>   3rdparty/libprocess/include/Makefile.am 
> d57a6103881aefef420dc0cf2e85cdf37d560887 
>   3rdparty/libprocess/include/process/state_machine.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> b01bf9784563a8f06168f8471222d8da2ffb9b85 
>   3rdparty/libprocess/src/tests/state_machine_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/63942/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 63911: Bumped up the number of args allowed in some libprocess templates.

2017-11-19 Thread Jie Yu 

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


Ship it!




Ship It!

- Jie Yu


On Nov. 17, 2017, 1:07 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63911/
> ---
> 
> (Updated Nov. 17, 2017, 1:07 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is a workaround until MESOS-7195 is fixed.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/async.hpp 
> a883f8835a7536bd218c5bc2b73e58bfecf82b5c 
>   3rdparty/libprocess/include/process/defer.hpp 
> 9457080a638d7bf444504b82629a7bc6b2de4116 
>   3rdparty/libprocess/include/process/deferred.hpp 
> af65eadee6f29a5bcd29b5c8ccf7444f39f8899a 
>   3rdparty/libprocess/include/process/delay.hpp 
> 13435a193e62851868c573481f43995498b85984 
>   3rdparty/libprocess/include/process/dispatch.hpp 
> 096cae037aec29059e1f13ac467bffe94ecd14ba 
>   3rdparty/libprocess/include/process/run.hpp 
> 1540a78e52a90d4c1d4165c46be353caaad21bce 
> 
> 
> Diff: https://reviews.apache.org/r/63911/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 63577: Fixed a task status update race in default executor tests.

2017-11-19 Thread Qian Zhang 

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

(Updated Nov. 20, 2017, 10:46 a.m.)


Review request for mesos, Alexander Rukletsov and Gaston Kleiman.


Changes
---

Addressed review comments.


Repository: mesos


Description
---

Previously in the test `DefaultExecutorTest.KillMultipleTasks` and
`DefaultExecutorTest.KillTaskGroupOnTaskFailure`, when launching a
task group which has multiple tasks, we expected the scheduler will
receive all the TASK_STARTING status updates before receiving any
TASK_RUNNING status updates. However this is not guaranteed, e.g.,
it is possible for the scheduler to receive TASK_RUNNING for the
first task before receiving TASK_STARTING for the second task.

So in this patch, we used `Sequence` to guarantee the order of
TASK_STARTING and TASK_RUNNING for each task but do not care about
the order between tasks.

The following 3 tests have their own solutions to handle this issue,
in this patch, I updated them to use the above solution.
  `DefaultExecutorTest.KillTask`
  `DefaultExecutorTest.CommitSuicideOnKillTask`
  `DefaultExecutorTest.ROOT_ContainerStatusForTask`


Diffs (updated)
-

  src/tests/default_executor_tests.cpp 65150212ae36094325c4f7f12b6ef135cd953170 
  src/tests/mesos.hpp 345b883a8c629bf5bed83e9236632c277f2eb0eb 


Diff: https://reviews.apache.org/r/63577/diff/4/

Changes: https://reviews.apache.org/r/63577/diff/3-4/


Testing
---

This patch touched 5 tests:
1. KillTask
2. KillMultipleTasks
3. KillTaskGroupOnTaskFailure
4. CommitSuicideOnKillTask
5. ROOT_ContainerStatusForTask

I ran each of them repeatedly (20 times), and all of them succeeded.


Thanks,

Qian Zhang

Review Request 63943: Renamed `TaskStatusEq()` to `TaskStatusTaskIdEq()`.

2017-11-19 Thread Qian Zhang 

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

Review request for mesos, Alexander Rukletsov and Gaston Kleiman.


Repository: mesos


Description
---

Renamed `TaskStatusEq()` to `TaskStatusTaskIdEq()`.


Diffs
-

  src/tests/master_tests.cpp 9c450b9f592d9e09a468f537d9b500e97acc636b 
  src/tests/mesos.hpp 345b883a8c629bf5bed83e9236632c277f2eb0eb 


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


Testing
---

Manually ran the test `MasterTest.MasterFailoverLongLivedExecutor` repeatedly 
(20 times), it succeeded.


Thanks,

Qian Zhang

Re: Review Request 63577: Fixed a task status update race in default executor tests.

2017-11-19 Thread Qian Zhang 


> On Nov. 16, 2017, 7:45 p.m., Alexander Rukletsov wrote:
> > src/tests/default_executor_tests.cpp
> > Lines 364-372 (patched)
> > 
> >
> > Indentation again. Could you please carefully check other places in the 
> > patch?

Sure, thanks for catching this!


> On Nov. 16, 2017, 7:45 p.m., Alexander Rukletsov wrote:
> > src/tests/default_executor_tests.cpp
> > Lines 926-928 (original), 1027-1029 (patched)
> > 
> >
> > What happens if a scheduler gets `TASK_FAILED` status update before 
> > destruction? Will the test fail with an "uninteresting mock function call"? 
> > Or there will be no failure because the expectation are specific to the 
> > state and we just get a warning in the log? I believe the latter, but could 
> > you please check this?

Can you please elaborate a bit about why the scheduler will get `TASK_FAILED` 
status update?


> On Nov. 16, 2017, 7:45 p.m., Alexander Rukletsov wrote:
> > src/tests/default_executor_tests.cpp
> > Line 1093 (original), 1195-1196 (patched)
> > 
> >
> > Not sure you need this comment now, the code is self describing. If you 
> > want to keep it, please add the same comment to all cases above : )

Agree, let me remove it.


> On Nov. 16, 2017, 7:45 p.m., Alexander Rukletsov wrote:
> > src/tests/mesos.hpp
> > Lines 2935-2941 (original), 2935-2941 (patched)
> > 
> >
> > This guy does the same as the one below, but for the old driver. Let's 
> > rename it to `TaskStatusStateEq` for consistency (in a separate patch).
> > 
> > Also, I'm not sure we need this long comment. Let's keep the first 
> > sentence only.
> > 
> > And s/task/taskInfo

Posted a separate patch: https://reviews.apache.org/r/63943

BTW, I rename it to `TaskStatusTaskIdEq` since it is used to match task ID 
rather than state.


- Qian


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


On Nov. 15, 2017, 2:21 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63577/
> ---
> 
> (Updated Nov. 15, 2017, 2:21 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Gaston Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously in the test `DefaultExecutorTest.KillMultipleTasks` and
> `DefaultExecutorTest.KillTaskGroupOnTaskFailure`, when launching a
> task group which has multiple tasks, we expected the scheduler will
> receive all the TASK_STARTING status updates before receiving any
> TASK_RUNNING status updates. However this is not guaranteed, e.g.,
> it is possible for the scheduler to receive TASK_RUNNING for the
> first task before receiving TASK_STARTING for the second task.
> 
> So in this patch, we used `Sequence` to guarantee the order of
> TASK_STARTING and TASK_RUNNING for each task but do not care about
> the order between tasks.
> 
> The following 3 tests have their own solutions to handle this issue,
> in this patch, I updated them to use the above solution.
>   `DefaultExecutorTest.KillTask`
>   `DefaultExecutorTest.CommitSuicideOnKillTask`
>   `DefaultExecutorTest.ROOT_ContainerStatusForTask`
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> 65113985acc0a8fac00374b074ef568aaa22c7ac 
>   src/tests/mesos.hpp e25defeb55608136e77363aa48cf820092a13a59 
> 
> 
> Diff: https://reviews.apache.org/r/63577/diff/3/
> 
> 
> Testing
> ---
> 
> This patch touched 5 tests:
> 1. KillTask
> 2. KillMultipleTasks
> 3. KillTaskGroupOnTaskFailure
> 4. CommitSuicideOnKillTask
> 5. ROOT_ContainerStatusForTask
> 
> I ran each of them repeatedly (20 times), and all of them succeeded.
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 63652: Added d_type check in containerizer backend validation.

2017-11-19 Thread Mesos Reviewbot Windows 

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



PASS: Mesos patch 63652 was successfully built and tested.

Reviews applied: `['63652']`

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

- Mesos Reviewbot Windows


On Nov. 19, 2017, 11:49 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63652/
> ---
> 
> (Updated Nov. 19, 2017, 11:49 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and James Peach.
> 
> 
> Bugs: MESOS-8121
> https://issues.apache.org/jira/browse/MESOS-8121
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In unified containerizer, the d_type cannot be 1
> if we are using the overlay fs backend.
> Raise error if user specifies overlayfs as backend.
> Fallback to other backends in the default case and
> raise a warning.
> 
> 
> Diffs
> -
> 
>   src/linux/fs.hpp cbc8bf79083ce2bc34fa698808eaf92764a577a9 
>   src/linux/fs.cpp 40c31a3ad2ccbeb59868c5e2d7b1809c613da8de 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 450a3b32d69d2882973a6ed4e94e169a0256056b 
> 
> 
> Diff: https://reviews.apache.org/r/63652/diff/5/
> 
> 
> Testing
> ---
> 
> Manually tested slave creation with default and overlayfs backends on XFS 
> based work_dir with different ftype mount options.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 63941: Added support for enum's to hashset and hashmap.

2017-11-19 Thread Benjamin Mahler 

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


Fix it, then Ship it!





3rdparty/stout/include/stout/hashmap.hpp
Lines 34 (patched)


A little odd that we're depending on it from the hashset header.



3rdparty/stout/include/stout/hashset.hpp
Lines 36 (patched)


Couldn't figure out how you came up with size_t here. Maybe mention why 
this is big enough and signedness doesn't matter?


- Benjamin Mahler


On Nov. 20, 2017, 1:02 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63941/
> ---
> 
> (Updated Nov. 20, 2017, 1:02 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for enum's to hashset and hashmap.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/hashmap.hpp 
> 539bbfd9250f2f372c2b3211e2379ad2229fa35e 
>   3rdparty/stout/include/stout/hashset.hpp 
> d2f7d5ddba62cc87e2b61cb06c86b8ef4cd1a212 
> 
> 
> Diff: https://reviews.apache.org/r/63941/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Review Request 63942: Added a state machine abstraction.

2017-11-19 Thread Benjamin Hindman 

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

An actor is often implemented like a state machine but the actual
management of the state and transitions is very manual. This
abstraction helps reduce a lot of the boilerplate assocaited with
state machines.


Diffs
-

  3rdparty/libprocess/Makefile.am 03a0ca87f31744c716c99e05aa07242fed480675 
  3rdparty/libprocess/include/Makefile.am 
d57a6103881aefef420dc0cf2e85cdf37d560887 
  3rdparty/libprocess/include/process/state_machine.hpp PRE-CREATION 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
b01bf9784563a8f06168f8471222d8da2ffb9b85 
  3rdparty/libprocess/src/tests/state_machine_tests.cpp PRE-CREATION 


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


Testing
---

make check


Thanks,

Benjamin Hindman

Review Request 63941: Added support for enum's to hashset and hashmap.

2017-11-19 Thread Benjamin Hindman 

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Added support for enum's to hashset and hashmap.


Diffs
-

  3rdparty/stout/include/stout/hashmap.hpp 
539bbfd9250f2f372c2b3211e2379ad2229fa35e 
  3rdparty/stout/include/stout/hashset.hpp 
d2f7d5ddba62cc87e2b61cb06c86b8ef4cd1a212 


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


Testing
---

make check


Thanks,

Benjamin Hindman

Re: Review Request 61149: Added stringification support for Future.

2017-11-19 Thread Benjamin Mahler 

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




3rdparty/libprocess/include/process/future.hpp
Lines 1782-1804 (patched)


Something like this?

```
  const std::string suffix = future.data->discard ? " (with discard)" : "";

  switch (future.data->state) {
case Future::PENDING:
  if (future.data->abandoned) {
return stream << "Abandoned" << suffix;
  }
  return stream << "Pending" << suffix;

case Future::READY:
  // TODO(benh): Stringify `Future::get()` if it can be
  // stringified (will need to be SFINAE'ed appropriately).
  return stream << "Ready" << suffix;

case Future::FAILED:
  return stream << "Failed" << suffix << ": " << future.failure();

case Future::DISCARDED:
  return stream << "Discarded" << suffix;
  }
```


- Benjamin Mahler


On July 27, 2017, 1:55 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61149/
> ---
> 
> (Updated July 27, 2017, 1:55 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is especially useful when creating error messages and the future
> has failed or been discarded.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/future.hpp 
> a11a588941b02d776da2f563dd246a92dfbbc360 
>   3rdparty/libprocess/src/tests/future_tests.cpp 
> 76a32bd69499e52ea1623ab982d65e1c7a0cbd32 
> 
> 
> Diff: https://reviews.apache.org/r/61149/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 61149: Added stringification support for Future.

2017-11-19 Thread Benjamin Mahler 

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


Ship it!





3rdparty/libprocess/include/process/future.hpp
Lines 1785-1786 (patched)


Any reason not to?

```
return stream << "Abandoned" << suffix;
```



3rdparty/libprocess/include/process/future.hpp
Lines 1790-1792 (patched)


Likewise these cases could be more succinct with returns and no breaks?

```
case Future::PENDING:
  return stream << "Pending" << suffix;
case ...
```


- Benjamin Mahler


On July 27, 2017, 1:55 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61149/
> ---
> 
> (Updated July 27, 2017, 1:55 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is especially useful when creating error messages and the future
> has failed or been discarded.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/future.hpp 
> a11a588941b02d776da2f563dd246a92dfbbc360 
>   3rdparty/libprocess/src/tests/future_tests.cpp 
> 76a32bd69499e52ea1623ab982d65e1c7a0cbd32 
> 
> 
> Diff: https://reviews.apache.org/r/61149/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 63940: Added a Future constructor for Try<Future>.

2017-11-19 Thread Benjamin Hindman 

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


Ship it!




Ship It!

- Benjamin Hindman


On Nov. 20, 2017, 12:23 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63940/
> ---
> 
> (Updated Nov. 20, 2017, 12:23 a.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Longer term we may want to figure out an alternative "flattening"
> strategy, but for now the approach has been to add constructors
> that allow flattening.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/future.hpp 
> 4cf44ba1a7052d5f84beb8fd6914b8e9e0437c0a 
>   3rdparty/libprocess/src/tests/future_tests.cpp 
> 76a32bd69499e52ea1623ab982d65e1c7a0cbd32 
> 
> 
> Diff: https://reviews.apache.org/r/63940/diff/1/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 63940: Added a Future constructor for Try<Future>.

2017-11-19 Thread Benjamin Mahler 

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

Review request for mesos and Benjamin Hindman.


Repository: mesos


Description
---

Longer term we may want to figure out an alternative "flattening"
strategy, but for now the approach has been to add constructors
that allow flattening.


Diffs
-

  3rdparty/libprocess/include/process/future.hpp 
4cf44ba1a7052d5f84beb8fd6914b8e9e0437c0a 
  3rdparty/libprocess/src/tests/future_tests.cpp 
76a32bd69499e52ea1623ab982d65e1c7a0cbd32 


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


Testing
---

Added a test.


Thanks,

Benjamin Mahler

Re: Review Request 63765: Fixed an issue with the scheduler driver subscribe backoff time.

2017-11-19 Thread Meng Zhu 


> On Nov. 18, 2017, 11:06 a.m., Vinod Kone wrote:
> > src/tests/scheduler_driver_tests.cpp
> > Lines 96 (patched)
> > 
> >
> > we use camel case for our variable names in mesos and snake case in 
> > libprocess/stout
> > 
> > s/in_sequence/inSequence/
> > 
> > btw, why is the sequence here necessary?

I want to make sure a call gets dropped first and then check expectation (no 
future calls). I think without the sequence, we can not enforce this order 
consistently?


- Meng


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


On Nov. 18, 2017, 1:32 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63765/
> ---
> 
> (Updated Nov. 18, 2017, 1:32 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Vinod Kone.
> 
> 
> Bugs: MESOS-8171
> https://issues.apache.org/jira/browse/MESOS-8171
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When framework failover time is set to zero (which is the
> default value), the scheduler driver subscribe backoff time
> will also become zero. Ignore failover time if it is zero
> when deciding the subscribe backoff time.
> Also added a dedicated test.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp 6028499285ad092ffd252e842c5d9835dd4442f8 
>   src/tests/scheduler_driver_tests.cpp 
> 14d872b8fadfd4ef16d8923fb0df924331534bc3 
> 
> 
> Diff: https://reviews.apache.org/r/63765/diff/2/
> 
> 
> Testing
> ---
> 
> make check, and the new dedicated test.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 63652: Added d_type check in containerizer backend validation.

2017-11-19 Thread Meng Zhu 

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

(Updated Nov. 19, 2017, 3:49 p.m.)


Review request for mesos, Gilbert Song and James Peach.


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


Repository: mesos


Description
---

In unified containerizer, the d_type cannot be 1
if we are using the overlay fs backend.
Raise error if user specifies overlayfs as backend.
Fallback to other backends in the default case and
raise a warning.


Diffs (updated)
-

  src/linux/fs.hpp cbc8bf79083ce2bc34fa698808eaf92764a577a9 
  src/linux/fs.cpp 40c31a3ad2ccbeb59868c5e2d7b1809c613da8de 
  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
450a3b32d69d2882973a6ed4e94e169a0256056b 


Diff: https://reviews.apache.org/r/63652/diff/5/

Changes: https://reviews.apache.org/r/63652/diff/4-5/


Testing
---

Manually tested slave creation with default and overlayfs backends on XFS based 
work_dir with different ftype mount options.


Thanks,

Meng Zhu

Re: Review Request 63652: Added d_type check in containerizer backend validation.

2017-11-19 Thread James Peach 


> On Nov. 14, 2017, 5:32 p.m., James Peach wrote:
> > src/slave/containerizer/mesos/provisioner/provisioner.cpp
> > Lines 154 (patched)
> > 
> >
> > The caller is responsible for logging the error. Let's be consistent 
> > and remove this `LOG`.
> 
> Meng Zhu wrote:
> I am worrying that, in the case of default backend selection, if the 
> underlying, say XFS, does not support `d_type`, the provisioner will just 
> silently eat the error message (near line 275) and fall back to a less ideal 
> backend. The user would have no clue of what happened. Putting the warning 
> message here is a simple thing that we can do to give some hint to the user.
> 
> What do you think?

Ah I see your point. For this support, let's not make this specific check 
special. When we are validating a back end, we can log the reason why a backend 
validation failed. Since this log only happens at startup, `INFO` seems 
appropriate after the call to `validateBackend`.

For example:
```
LOG(INFO) << "Provisioner backend '"<< backend << "' is not supported on '"
  << rootDir.get << "': " << support.error();
```


- James


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


On Nov. 14, 2017, 9:30 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63652/
> ---
> 
> (Updated Nov. 14, 2017, 9:30 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and James Peach.
> 
> 
> Bugs: MESOS-8121
> https://issues.apache.org/jira/browse/MESOS-8121
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In unified containerizer, the d_type cannot be 1
> if we are using the overlay fs backend.
> Raise error if user specifies overlayfs as backend.
> Fallback to other backends in the default case and
> raise a warning.
> 
> 
> Diffs
> -
> 
>   src/linux/fs.hpp cbc8bf79083ce2bc34fa698808eaf92764a577a9 
>   src/linux/fs.cpp 40c31a3ad2ccbeb59868c5e2d7b1809c613da8de 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 450a3b32d69d2882973a6ed4e94e169a0256056b 
> 
> 
> Diff: https://reviews.apache.org/r/63652/diff/4/
> 
> 
> Testing
> ---
> 
> Manually tested slave creation with default and overlayfs backends on XFS 
> based work_dir with different ftype mount options.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 63910: Added 3 tests for TCP/HTTP(S) health check support for Docker container.

2017-11-19 Thread Mesos Reviewbot Windows 

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



PASS: Mesos patch 63910 was successfully built and tested.

Reviews applied: `['63794', '63795', '63796', '63910']`

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

- Mesos Reviewbot Windows


On Nov. 19, 2017, 1:41 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63910/
> ---
> 
> (Updated Nov. 19, 2017, 1:41 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Avinash sridharan.
> 
> 
> Bugs: MESOS-8050
> https://issues.apache.org/jira/browse/MESOS-8050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed 3 tests which only run on IPv4 Docker network.
>   ROOT_DOCKER_DockerHealthyTaskViaHTTP
>   ROOT_DOCKER_DockerHealthyTaskViaHTTPS
>   ROOT_DOCKER_DockerHealthyTaskViaTCP
> 
> Added 3 tests which run on both IPv4 and IPv6 Docker networks, so they
> should cover the above 3 tests.
>   ROOT_DOCKER_USERNETWORK_HealthyTaskViaHTTP
>   ROOT_DOCKER_USERNETWORK_HealthyTaskViaHTTPS
>   ROOT_DOCKER_USERNETWORK_HealthyTaskViaTCP
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 67945ddc4f98ffa072f584af8106967e7ff336d3 
>   src/tests/health_check_tests.cpp c0dcba265363f2149b217b189ee5a8e925e40ea1 
>   src/tests/mesos.hpp 345b883a8c629bf5bed83e9236632c277f2eb0eb 
> 
> 
> Diff: https://reviews.apache.org/r/63910/diff/2/
> 
> 
> Testing
> ---
> 
> Ran all newly added tests repeatedly (20 times), they all succeeded.
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 63910: Added 3 tests for TCP/HTTP(S) health check support for Docker container.

2017-11-19 Thread Qian Zhang 

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

(Updated Nov. 19, 2017, 9:41 p.m.)


Review request for mesos, Alexander Rukletsov and Avinash sridharan.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Removed 3 tests which only run on IPv4 Docker network.
  ROOT_DOCKER_DockerHealthyTaskViaHTTP
  ROOT_DOCKER_DockerHealthyTaskViaHTTPS
  ROOT_DOCKER_DockerHealthyTaskViaTCP

Added 3 tests which run on both IPv4 and IPv6 Docker networks, so they
should cover the above 3 tests.
  ROOT_DOCKER_USERNETWORK_HealthyTaskViaHTTP
  ROOT_DOCKER_USERNETWORK_HealthyTaskViaHTTPS
  ROOT_DOCKER_USERNETWORK_HealthyTaskViaTCP


Diffs (updated)
-

  src/tests/containerizer/docker_containerizer_tests.cpp 
67945ddc4f98ffa072f584af8106967e7ff336d3 
  src/tests/health_check_tests.cpp c0dcba265363f2149b217b189ee5a8e925e40ea1 
  src/tests/mesos.hpp 345b883a8c629bf5bed83e9236632c277f2eb0eb 


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

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


Testing
---

Ran all newly added tests repeatedly (20 times), they all succeeded.


Thanks,

Qian Zhang

Re: Review Request 63910: Added 3 tests for TCP/HTTP(S) health check support for Docker container.

2017-11-19 Thread Qian Zhang 


> On Nov. 18, 2017, 3:25 a.m., Avinash sridharan wrote:
> > src/tests/health_check_tests.cpp
> > Lines 1972 (patched)
> > 
> >
> > What is `local mode`? :)

That is not something added or modified in this patch :-) In 
`src/tests/health_check_tests.cpp`, I only removed 3 existing tests, and added 
3 new tests, but did not change any other tests, so I think it is better to 
leave the unrelated tests untouched in this patch. I am not sure why the code 
diff of this patch looks in this way, sorry for the confusion.


> On Nov. 18, 2017, 3:25 a.m., Avinash sridharan wrote:
> > src/tests/health_check_tests.cpp
> > Lines 2040 (patched)
> > 
> >
> > Feels like we should have a helper function for this, similar to 
> > `createTask`?

Ditto.


> On Nov. 18, 2017, 3:25 a.m., Avinash sridharan wrote:
> > src/tests/health_check_tests.cpp
> > Line 2043 (original), 2050 (patched)
> > 
> >
> > why s/->/get()?

Ditto.


> On Nov. 18, 2017, 3:25 a.m., Avinash sridharan wrote:
> > src/tests/health_check_tests.cpp
> > Line 2062 (original), 2070 (patched)
> > 
> >
> > Not yours but s/mesos/Mesos

Ditto.


> On Nov. 18, 2017, 3:25 a.m., Avinash sridharan wrote:
> > src/tests/health_check_tests.cpp
> > Line 2080 (original), 2077 (patched)
> > 
> >
> > s/CMD/command

Ditto.


> On Nov. 18, 2017, 3:25 a.m., Avinash sridharan wrote:
> > src/tests/health_check_tests.cpp
> > Line 2268 (original), 2308 (patched)
> > 
> >
> > This test is a user network test, so we technically don't need to do 
> > this. We could set the port statically since the container is going to get 
> > its own network namespace.

Good catch!


> On Nov. 18, 2017, 3:25 a.m., Avinash sridharan wrote:
> > src/tests/health_check_tests.cpp
> > Line 2272 (original), 2312 (patched)
> > 
> >
> > Just a thought: Can we just use `nginx:alpine` image? We won't need to 
> > rely on `nc` to do this stuff. It would work cleanly since the container is 
> > on its network namespace.

I thought about that before, but unfortunately I found `nginx:alpine` can only 
work over IPv4 even IPv6 is enabled in the Docker container.


> On Nov. 18, 2017, 3:25 a.m., Avinash sridharan wrote:
> > src/tests/health_check_tests.cpp
> > Lines 2450 (patched)
> > 
> >
> > We should try and see if we can publish this image to 
> > :https://hub.docker.com/u/mesos/
> > 
> > Also, I assuming this a custorm image. We should commit the 
> > `Dockerfile` for this image to our test repo. More importantly want to the 
> > cert being used in this test to be part of the repo.

Agree, do you know who has the permission to publish image to 
https://hub.docker.com/u/mesos/? And for the Dockerfile & the cert (and even 
the Python code `https_server.py`), I think we can just put them in the 
description of the Docker image so that we can see all of them in the same 
place.


> On Nov. 18, 2017, 3:25 a.m., Avinash sridharan wrote:
> > src/tests/health_check_tests.cpp
> > Lines 2525 (patched)
> > 
> >
> > As you mentioned, given that this test is identical to the HTTP test 
> > above, can we just parameterize the `HealthCheck.type` field as well in the 
> > above test case?

Yeah, I thought about that before, but the problem is, besides 
`ROOT_DOCKER_USERNETWORK_HealthyTaskViaHTTP` and 
`ROOT_DOCKER_USERNETWORK_HealthyTaskViaTCP`, we have another test 
`ROOT_DOCKER_USERNETWORK_HealthyTaskViaHTTPS`, I am not sure how it can work if 
we parameterize the `HealthCheck.type` field as well.


- Qian


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


On Nov. 17, 2017, 9:35 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63910/
> ---
> 
> (Updated Nov. 17, 2017, 9:35 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Avinash sridharan.
> 
> 
> Bugs: MESOS-8050
> https://issues.apache.org/jira/browse/MESOS-8050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed 3 tests which only run on IPv4

Re: Review Request 63939: Added a script to build Mesos docker image.

2017-11-19 Thread Mesos Reviewbot Windows 

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



PASS: Mesos patch 63939 was successfully built and tested.

Reviews applied: `['63939']`

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

- Mesos Reviewbot Windows


On Nov. 19, 2017, 6:28 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63939/
> ---
> 
> (Updated Nov. 19, 2017, 6:28 a.m.)
> 
> 
> Review request for mesos and Kapil Arya.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This script leverages the packaging scripts to build the RPM first, and
> then generate the Mesos docker image by installing the Mesos RPM. This
> results in smaller Mesos docker image.
> 
> This script can be run on both Linux or OSX. You just need to have
> Docker installed.
> 
> Currently, the script will build a Mesos docker image from the current
> head. Future work will make it work for building a specific tag or sha.
> 
> 
> Diffs
> -
> 
>   support/packaging/centos/build-docker-image.sh PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/63939/diff/1/
> 
> 
> Testing
> ---
> 
> Tested on CentOS7 and OSX
> 
> 
> Thanks,
> 
> Jie Yu
> 
>