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

Ship it!

Was great sitting down and going over all of this stuff with you over the past 
week, thanks for the patience! Just some final adjustments that we discussed 
based on pulling this down and getting ready to commit.

3rdparty/libprocess/include/process/authenticator.hpp (line 52)

    Let's maybe say here that this allows us to implement different 
authenticators based on the scheme and include the examples below (Basic, 
Digest, SPNEGO, etc).

3rdparty/libprocess/include/process/authenticator.hpp (line 65)

    Whoops, stale now that you have the namespace.

3rdparty/libprocess/include/process/http.hpp (lines 545 - 552)

    As we discussed, should we have a TODO to remove these?

3rdparty/libprocess/src/authentication_router.hpp (lines 35 - 37)

    A little bit stale now that we called this router instead of realm manager, 

3rdparty/libprocess/src/authentication_router.hpp (line 48)

    How about unset for symettry? I see why you called this 'set' and 'unset' 
and that does describe the single authenticator per-realm constraint. Thinking 
over this again though, let's also add a small comment to describe this 

3rdparty/libprocess/src/authentication_router.hpp (line 57)

    Whoops, "either" here is orphaned.

3rdparty/libprocess/src/authentication_router.hpp (line 69)

    Whoops, stale comment.

3rdparty/libprocess/src/authentication_router.cpp (line 96)

    Hm.. perhaps we also need something here to mention that we're assuming 
absolute paths, which is ensured by libprocess. Should consider maybe returning 
a failure here when a relative path is detected.

3rdparty/libprocess/src/authentication_router.cpp (line 107)

    Whoops, should have a space before the brace here.

3rdparty/libprocess/src/authentication_router.cpp (line 111)

    Perhaps we should handle the "error" logging case first, as that tends to 
be how we structure the code and we can avoid an else more clearly

3rdparty/libprocess/src/authentication_router.cpp (lines 114 - 116)

    How about we validate that the Result is valid before we send it up to 
libprocess? We can CHECK validity in libprocess since we control the code here, 
and avoid having to do validity checking (which we weren't really doing 

3rdparty/libprocess/src/authentication_router.cpp (line 136)

    Why the 'if' here? 'process' should not be null here, unless there is a 
bug. We could add a check but for now I'll just remove the 'if' guard to be 
consistent with the rest of the process wrapper code FWICT.

3rdparty/libprocess/src/process.cpp (line 60)

    What was this for? The result? Hm.. should be ok in this case to just 
include the router since we are only using signatures from the router.

3rdparty/libprocess/src/process.cpp (line 109)


3rdparty/libprocess/src/process.cpp (line 128)

    We should consider just making this authentication::Router, but I'll leave 
it for now.

3rdparty/libprocess/src/process.cpp (line 953)

    Even though there whould probably be just one comment above all these 
saying we're creating the globals, let's add another comment here to be 
consistent with the current code:
    // Create the global HTTP authentication router.
    authentication_router = new AuthenticationRouter();

3rdparty/libprocess/src/process_reference.hpp (line 52)

    This should be in a separate patch, much like we did for the promise 
setting bug we noticed.

- Ben Mahler

On Nov. 18, 2015, 10:43 a.m., Alexander Rojas wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> -----------------------------------------------------------
> (Updated Nov. 18, 2015, 10:43 a.m.)
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> Bugs: MESOS-3231
>     https://issues.apache.org/jira/browse/MESOS-3231
> Repository: mesos
> Description
> -------
> The Authenticator interface allows us to implement different
> authenticators based on the scheme (e.g. Basic, Digest, SPNEGO).
> The AuthenticationRouter manages the authentication realms and
> the mapping from endpoints to realms. It is then used by the
> ProcessManager to route requests to the authenticator for the
> realm, if applicable.
> Diffs
> -----
>   3rdparty/libprocess/Makefile.am cdefa37528ea69422978a9772f955042e882dde4 
>   3rdparty/libprocess/include/Makefile.am 
> e6be2c4db121585bbe7f3c0627de048adc7cfb2c 
>   3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/event.hpp 
> 28ce1928877084f0e1a73fdad789224c86e53f46 
>   3rdparty/libprocess/include/process/http.hpp 
> 90c9be122ee0c402b806d70fc818e3c03b15101a 
>   3rdparty/libprocess/src/CMakeLists.txt 
> fb9bd04832779ac43151f2feb3dfbf58cb996434 
>   3rdparty/libprocess/src/authentication_router.hpp PRE-CREATION 
>   3rdparty/libprocess/src/authentication_router.cpp PRE-CREATION 
>   3rdparty/libprocess/src/process.cpp 
> 7abdf21a5784920251c3627f9820c12fdc356c6e 
>   3rdparty/libprocess/src/process_reference.hpp 
> e6110bba2a54948be68e58ab9de988565b7d95a8 
> Diff: https://reviews.apache.org/r/37999/diff/
> Testing
> -------
> make check
> Thanks,
> Alexander Rojas

Reply via email to