----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38094/#review112650 -----------------------------------------------------------
Ship it! Looks good, just some minor items below that I'll address before committing. Notably there was a redundant if condition left after the change to use .get()'s Option. 3rdparty/libprocess/include/process/authenticator.hpp (line 91) <https://reviews.apache.org/r/38094/#comment173167> Not necessary, but should we put the virtual here? 3rdparty/libprocess/src/authenticator.cpp (line 22) <https://reviews.apache.org/r/38094/#comment173160> Where did this come from? 3rdparty/libprocess/src/authenticator.cpp (lines 34 - 36) <https://reviews.apache.org/r/38094/#comment173165> We're already in the http namespace, did you need these? 3rdparty/libprocess/src/authenticator.cpp (lines 71 - 73) <https://reviews.apache.org/r/38094/#comment173161> This case is redundant with your credentials.isNone() check below? 3rdparty/libprocess/src/authenticator.cpp (line 76) <https://reviews.apache.org/r/38094/#comment173163> There is a newline here between the variable declaration and the 'if' statement, but there isn't in two cases below, then the last case has a newline. How about we just use a newline in each case here? 3rdparty/libprocess/src/authenticator.cpp (line 82) <https://reviews.apache.org/r/38094/#comment173166> We should probably error out here if the size is not exactly 2, much like you did below for the credentials. 3rdparty/libprocess/src/authenticator.cpp (line 96) <https://reviews.apache.org/r/38094/#comment173164> Seems a bit odd to put a body here, but not in the other cases, no? - Ben Mahler On Jan. 4, 2016, 11:26 a.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38094/ > ----------------------------------------------------------- > > (Updated Jan. 4, 2016, 11:26 a.m.) > > > Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till > Toenshoff. > > > Bugs: MESOS-3232 > https://issues.apache.org/jira/browse/MESOS-3232 > > > Repository: mesos > > > Description > ------- > > See summary. > > > Diffs > ----- > > 3rdparty/libprocess/Makefile.am 6749fd40e672c3626f6605756bd787a878271034 > 3rdparty/libprocess/include/process/authenticator.hpp > 5a32e9a38a0bec7aa3faef23b792f3bf3d659d4f > 3rdparty/libprocess/src/CMakeLists.txt > 681f0cfec57e152568da41698c8bdd52c05f65a6 > 3rdparty/libprocess/src/authenticator.cpp PRE-CREATION > 3rdparty/libprocess/src/tests/http_tests.cpp > ec7b4aa90bf7024469bf9774e01e46c9f7fd094f > > Diff: https://reviews.apache.org/r/38094/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Alexander Rojas > >
