This is an automatically generated e-mail. To reply, visit:

Great stuff, happy to see it landing! Some general suggestions, not necessarily 
for immediate execution, but for posterity (and maybe follow-up JIRAs ; ):

1. Smaller, self-contained reviews. For example, you can update `Unauthorized` 
-> `Forbidden` in a separate review for clarity and to keep this one smaller.
2. When updating endpoints code, always check and update appropriate 
documentation. For example `Unauthorized` -> `Forbidden` must be reflected in 
reservation and persistent volumes docs.
3. Spooky places like blobs that are hard to read should ideally be prepended 
with a TODO saying you plan to refactor it. If it does not look nice and slick 
to you, chances are the next guy will feel the same. Let's show we care about 
that and are not leaving broken windows.
4. Changes touching libprocess are vulnerable to races, especially in tests, 
where the libprocess context may be shared among multiple masters and agents. 
Please extend the testing (and mention this in the "Testing Done" section) with 
different platforms, and goodies like `--gtest_shuffle` and `--gtest-repeat`.

src/authentication/http/basic_authenticator_factory.cpp (line 40)

    #include <stout/foreach.hpp>?

src/master/flags.cpp (lines 447 - 449)

    The variable name implies there can be many authenticators, while the 
comment says it may just be one. What's right? I assume the former, because you 
do a split on comma later in the patch. Mind mentioning that?

src/master/http.cpp (lines 1671 - 1672)

    I like how the scope is narrowed now and we do not need to carry around the 
unused secret!

src/master/master.hpp (lines 1179 - 1182)

    Let's remove this in a patch chained to this review (and fix MESOS-4149 as 
a consequence) in order not to forget.

src/master/master.cpp (lines 500 - 508)

    Let's add a TODO to refactor lines 500-556 into a factory so people see we 
consider such blobs a tech debt.

src/tests/mesos.cpp (lines 435 - 438)

    Since `setAuthenticator` is called in `Master::initialize()` won't it be a 
problem if we have multiple masters in a test case? Is `unsetAuthenticator()` 
idempotent? In tests multiple masters (and agents) share the same libprocess 
context (which is not true in production scenarios), I wonder whether this can 
be a problem.

src/tests/mesos.cpp (line 438)

    4 spaces, please.

src/tests/persistent_volume_endpoints_tests.cpp (lines 900 - 902)

    I suggest to make this change in a separate patch, but this fits one line 

- Alexander Rukletsov

On Jan. 7, 2016, 3:25 p.m., Alexander Rojas wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39043/
> -----------------------------------------------------------
> (Updated Jan. 7, 2016, 3:25 p.m.)
> Review request for mesos, Adam B, Bernd Mathiske, Isabel Jimenez, Jan 
> Schlicht, and Till Toenshoff.
> Bugs: MESOS-3756
>     https://issues.apache.org/jira/browse/MESOS-3756
> Repository: mesos
> Description
> -------
> 1. Adds a flag to load an HTTP Authenticator module from the flags.
> 2. If provided, uses the credentials file to initialize the default HTTP 
> Authenticator.
> 3. Updates the existing endpoints which implement their own basic HTTP 
> authenticator with the libprocess one.
> 4. Updates one test which expected the wrong results since now credentials 
> are checked before the body of the request.
> Diffs
> -----
>   include/mesos/authentication/http/basic_authenticator_factory.hpp 
>   src/authentication/http/basic_authenticator_factory.cpp PRE-CREATION 
>   src/master/constants.hpp f1624e1ed5091f08f57d4382b344cab8b05ba840 
>   src/master/constants.cpp 589f2ee55a24d7e8b9352a4c8f94a3785fd1bef4 
>   src/master/flags.hpp 9af6c68eef6bcf39d5776809fab6c66dc95da6b2 
>   src/master/flags.cpp f864419a6276356c97a0a4fe29e5cfce6c4660c4 
>   src/master/http.cpp d7afa2af989eebfc9039b3681f087ce570f601d5 
>   src/master/master.hpp f764915ff8bbf74b99a617e3c4735d38fc13e5f0 
>   src/master/master.cpp 40ce3e17fca88da689128bcf5d35fdddc396c011 
>   src/master/quota_handler.cpp 93960f3728f9e68c085e6009fe975542d29d2551 
>   src/tests/master_quota_tests.cpp bc8a117982b994279e0df7639b21abaeb308b76d 
>   src/tests/mesos.cpp 3867ed2fcb197f98f0a19ca37ed81392db27e8a0 
> Diff: https://reviews.apache.org/r/39043/diff/
> Testing
> -------
> make check
> Thanks,
> Alexander Rojas

Reply via email to