----------------------------------------------------------- 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) <https://reviews.apache.org/r/61157/#comment257478> 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) <https://reviews.apache.org/r/61157/#comment257474> Much cleaner! 3rdparty/libprocess/src/process.cpp Lines 2486-2487 (original) <https://reviews.apache.org/r/61157/#comment257475> Did you retain this VLOGging by having the proxy log failures? 3rdparty/libprocess/src/process.cpp Line 2507 (original), 2486 (patched) <https://reviews.apache.org/r/61157/#comment257476> Why did you want auto here? 3rdparty/libprocess/src/process.cpp Lines 2528-2535 (original), 2499-2502 (patched) <https://reviews.apache.org/r/61157/#comment257477> 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) <https://reviews.apache.org/r/61157/#comment257480> Hm.. it looks like you've already got this check above? Am I missing something? 3rdparty/libprocess/src/process.cpp Lines 2518-2519 (patched) <https://reviews.apache.org/r/61157/#comment257479> Hm.. should we log here or just log in the proxy? 3rdparty/libprocess/src/process.cpp Lines 2603-2604 (original), 2560-2561 (patched) <https://reviews.apache.org/r/61157/#comment257481> 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) <https://reviews.apache.org/r/61157/#comment257482> 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) <https://reviews.apache.org/r/61157/#comment257483> Just noticing that http2 is a very misleading name ;) 3rdparty/libprocess/src/tests/process_tests.cpp Lines 1723-1725 (patched) <https://reviews.apache.org/r/61157/#comment257484> 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 > >
