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

2018-01-15 Thread Benjamin Mahler 

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


Ship it!




Ship It!

- 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/61157/
> ---
> 
> (Updated July 27, 2017, 1:55 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored ProcessManager::handle for future use with http::Server.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/event.hpp 
> ec64eb786b6cf30bf03ba07a30c6e44d2db9173a 
>   3rdparty/libprocess/src/process.cpp 
> 2126c272a69bfa53b4b3cbbb1a55a3f81a5da8ad 
> 
> 
> Diff: https://reviews.apache.org/r/61157/diff/7/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

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

2017-11-27 Thread Benjamin Mahler 

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


Ship it!




Coming back to this change without context, it took some time to realize that 
this change was primarily about returning a `Future` from 
`ProcessManager::handle`. A commit description for posterity would be helpful 
IMO.


3rdparty/libprocess/src/process.cpp
Lines 829-831 (patched)


Looks like we can std::move in the future as well, but not until dmitry's 
patches land, not sure if you want to put down a TODO for this.



3rdparty/libprocess/src/process.cpp
Lines 2641-2642 (original), 2608-2609 (patched)


Can this be removed?


- 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/61157/
> ---
> 
> (Updated July 27, 2017, 1:55 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored ProcessManager::handle for future use with http::Server.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/event.hpp 
> 571ffb1ea5c4787f10c2f4dee249aadc8b904bc6 
>   3rdparty/libprocess/src/process.cpp 
> 64bcce215d19558dd493e30e96ca16577fe0722a 
> 
> 
> Diff: https://reviews.apache.org/r/61157/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

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 61157: Refactored ProcessManager::handle for future use with http::Server.

2017-08-03 Thread Benjamin Mahler 

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


Ship it!




Ship It!

- 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/61157/
> ---
> 
> (Updated July 27, 2017, 1:55 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored ProcessManager::handle for future use with http::Server.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> af5a75950bf24059d291acfd48399ab2d55eb8c2 
> 
> 
> Diff: https://reviews.apache.org/r/61157/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

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

2017-07-30 Thread Benjamin Mahler 

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



Great cleanup! Although I held off on a ship it because we ought to pull out 
the removal of Libprocess-From support into a distinct change.


3rdparty/libprocess/src/process.cpp
Lines 624-629 (original), 624-629 (patched)


In your change to remove support for the old responseless protocol (see my 
comment about pulling that out), we should also remove Libprocess-From from 
this?



3rdparty/libprocess/src/process.cpp
Line 2450 (original), 2461 (patched)


Much cleaner!



3rdparty/libprocess/src/process.cpp
Lines 2486-2487 (original)


Did you retain this VLOGging by having the proxy log failures?



3rdparty/libprocess/src/process.cpp
Line 2507 (original), 2486 (patched)


Why did you want auto here?



3rdparty/libprocess/src/process.cpp
Lines 2528-2535 (original), 2499-2502 (patched)


This seems too sneaky to me, can you pull it out and describe the removal 
of the support in its own commit?

Also, we should return a failure to the client telling it we don't support 
Libprocess-From if we find that header? Otherwise, we're sending Accepted to a 
libprocess that can't handle it?



3rdparty/libprocess/src/process.cpp
Line 2553 (original), 2515-2520 (patched)


Hm.. it looks like you've already got this check above? Am I missing 
something?



3rdparty/libprocess/src/process.cpp
Lines 2518-2519 (patched)


Hm.. should we log here or just log in the proxy?



3rdparty/libprocess/src/process.cpp
Lines 2603-2604 (original), 2560-2561 (patched)


As an aside, seems like we could really tidy up and make the logging 
consistent if we just logged these in the http proxy.



3rdparty/libprocess/src/tests/process_tests.cpp
Line 1330 (original), 1328-1331 (patched)


Can the two EQs be EXPECTs? You could use AWAIT_RESPONSE_STATUS_EQ to make 
this just one line.



3rdparty/libprocess/src/tests/process_tests.cpp
Line 1343 (original), 1344 (patched)


Just noticing that http2 is a very misleading name ;)



3rdparty/libprocess/src/tests/process_tests.cpp
Lines 1723-1725 (patched)


Can these be expects? Also, I think this can be one line per my above 
comment.


- 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/61157/
> ---
> 
> (Updated July 27, 2017, 1:55 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored ProcessManager::handle for future use with http::Server.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> b268cdad776a3ca2a87cbe60eb098bde2a70667c 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> ed11909a2a5e3214fa974bdea098f4057cea9666 
> 
> 
> Diff: https://reviews.apache.org/r/61157/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

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

2017-07-26 Thread Mesos Reviewbot Windows 

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



Bad patch!

Reviews applied: [61157, 61156, 61155, 61154, 61153, 61152, 61151, 61150, 
61149, 61148, 61147, 55324, 55323, 55322, 55321, 55320]

Failed command: python support/apply-reviews.py -n -r 55320

Error:
Traceback (most recent call last):
  File "support/apply-reviews.py", line 417, in 
main()
  File "support/apply-reviews.py", line 412, in main
reviewboard(options)
  File "support/apply-reviews.py", line 402, in reviewboard
apply_review(options)
  File "support/apply-reviews.py", line 158, in apply_review
fetch_patch(options)
  File "support/apply-reviews.py", line 191, in fetch_patch
context=ssl_create_default_context())
  File "C:\Python27\lib\urllib2.py", line 154, in urlopen
return opener.open(url, data, timeout)
  File "C:\Python27\lib\urllib2.py", line 435, in open
response = meth(req, response)
  File "C:\Python27\lib\urllib2.py", line 548, in http_response
'http', request, response, code, msg, hdrs)
  File "C:\Python27\lib\urllib2.py", line 473, in error
return self._call_chain(*args)
  File "C:\Python27\lib\urllib2.py", line 407, in _call_chain
result = func(*args)
  File "C:\Python27\lib\urllib2.py", line 556, in http_error_default
raise HTTPError(req.get_full_url(), code, msg, hdrs, fp)
urllib2.HTTPError: HTTP Error 404: NOT FOUND

Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/204/console

- Mesos Reviewbot Windows


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/61157/
> ---
> 
> (Updated July 27, 2017, 1:55 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored ProcessManager::handle for future use with http::Server.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> b268cdad776a3ca2a87cbe60eb098bde2a70667c 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> ed11909a2a5e3214fa974bdea098f4057cea9666 
> 
> 
> Diff: https://reviews.apache.org/r/61157/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

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

2017-07-26 Thread Benjamin Hindman 

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Refactored ProcessManager::handle for future use with http::Server.


Diffs
-

  3rdparty/libprocess/src/process.cpp b268cdad776a3ca2a87cbe60eb098bde2a70667c 
  3rdparty/libprocess/src/tests/process_tests.cpp 
ed11909a2a5e3214fa974bdea098f4057cea9666 


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


Testing
---

make check


Thanks,

Benjamin Hindman