----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50320/#review143188 -----------------------------------------------------------
Good progress, but let's get rid of the ostringstream and move `DEFAULT_HTTP_AUTHENTICATOR` into `common/http.hpp` src/common/http.hpp (lines 171 - 173) <https://reviews.apache.org/r/50320/#comment208986> Why is this a parameter when it's the same everywhere? I know it technically refers to two identically named constants on master+slave, but they also have the same value (and presumably always will). Let's move that constant into `src/common/http.hpp` too and use it directly in the helper/elsewhere. src/common/http.hpp (lines 179 - 181) <https://reviews.apache.org/r/50320/#comment208980> Why does a function called `initializeHttpAuthenticator()` (singular) take in a `vector<string>& authenticatorNames` (plural)? Either it should be `initializeHttpAuthenticators(authenticatorNames)` or `initializeHttpAuthenticator(authenticatorName)`. If you're going to do the "only 1 name allowed" check+error inside the helper, then name it `initializeHttpAuthenticators` (plural). src/common/http.cpp (line 38) <https://reviews.apache.org/r/50320/#comment208988> You shouldn't need to exit directly in this helper src/common/http.cpp (line 868) <https://reviews.apache.org/r/50320/#comment208989> We only use snake_case when protobuf forces us to. Otherwise we use camelCase. How about just calling this "error"? src/common/http.cpp (lines 875 - 877) <https://reviews.apache.org/r/50320/#comment208990> Why not just `return Error("Multiple HTTP authenticators not supported");`? In fact, you probably don't even need the error_stream at all if you're just using the string immediately after each time. `+` should work fine. src/common/http.cpp (line 881) <https://reviews.apache.org/r/50320/#comment208991> Just move the constant to common/http.hpp and use it here. No need for an extra Option check. src/common/http.cpp (lines 897 - 899) <https://reviews.apache.org/r/50320/#comment208987> return Error() - Adam B On July 21, 2016, 9:45 p.m., Zhitao Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50320/ > ----------------------------------------------------------- > > (Updated July 21, 2016, 9:45 p.m.) > > > Review request for mesos, Adam B and Greg Mann. > > > Bugs: MESOS-5851 > https://issues.apache.org/jira/browse/MESOS-5851 > > > Repository: mesos > > > Description > ------- > > Refactor common HTTP authenticator initialize into helper function. > > > Diffs > ----- > > src/common/http.hpp 2dfa789d475598f07a5123899025937fd145a3da > src/common/http.cpp d73170df4e35b84d194347406b3061236de6f7be > src/master/master.hpp 6decff6f4b9c3434de030fd5c06df4c683a7abad > src/master/master.cpp 370fd8712062dc75bb81824cb99ccc7920acbf78 > src/slave/slave.hpp ffe4220c9289419ab1b1a2c1f499b6eac3c01e4b > src/slave/slave.cpp 3e7131170e1f9bf682fb0c603d2ca39f514d87d9 > > Diff: https://reviews.apache.org/r/50320/diff/ > > > Testing > ------- > > `make check` > > > Thanks, > > Zhitao Li > >
