mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Adam B <a...@mesosphere.io>
Subject Re: Review Request 50320: Refactor common HTTP authenticator initialize into helper function.
Date Fri, 22 Jul 2016 07:10:07 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message