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

This is starting to shape up, thanks Alexander. As we discussed offline, just 
wanted to surface the main bits of feedback so you can start addressing them. I 
will follow up with feedback for the tests, wasn't able to do a complete pass 
in one sitting.

3rdparty/libprocess/include/process/process.hpp (lines 65 - 86)

    Why isn't this in the http header? It looks like firewall belongs there too 
since it's more aptly namespaced as 'http::Firewall'?

3rdparty/libprocess/include/process/process.hpp (lines 68 - 73)

    How about the following to be a bit more succinct?
     * Sets (or overwrites) the authenticator for the realm.
     * Every incoming HTTP request to an endpoint associated
     * with the realm will be authenticated with the given
     * authenticator.

3rdparty/libprocess/include/process/process.hpp (lines 79 - 82)

    Not sure we need to say "still", how about:
     * Unsets the authenticator for the realm.
     * Any endpoint mapped to the realm will no
     * longer be authenticated.

3rdparty/libprocess/include/process/process.hpp (lines 261 - 263)

    I'm guessing this is about introducing a type to make this more explicit in 
the signature, not type safety:
    TODO(arojas): Consider introducing an `authentication::Principal` type.
    Similarly, we probably need a TODO on top of the `route()` declarations for 
an `authentication::Realm` type?

3rdparty/libprocess/include/process/process.hpp (lines 261 - 264)

    Looks like this needs javadoc like the existing typedef, then you can 
remove the `/* principal */` that you've added here.

3rdparty/libprocess/include/process/process.hpp (lines 372 - 374)

    There is a typo in here :)
    Also, could you try to avoid the run on sentence? How about saying 
something about http request handlers are equivalent to authenticated http 
request handlers with a none principal, so we convert them.

3rdparty/libprocess/src/authentication_router.hpp (lines 52 - 56)

    The second sentence here confused me (e.g. the current implementation of 
what?), so how about we just write the first sentence and let the reader figure 
out the implications? Note that the implications could change over time so it's 
possible for it to go out of sync and cause more confusion.

3rdparty/libprocess/src/process.cpp (lines 231 - 232)

    We really need to help the reader understand this!
    // Http pipelining requires that when we perform http
    // authentication, we need to ensure that authentication
    // results are processed in sequential order. Otherwise,
    // we may send responses out-of-order!
    // The ordering needs to be respected within a Process, but
    // not across Processes since dispatches are asynchronous.
    // Note that this needs to be done explicitly here
    // because the authentication router does not guarantee
    // ordered completion of its Futures (it doesn't have the
    // knowledge of UPIDs necessary to do it in a fine-grained
    // manner).
    hashmap<UPID, Sequence> authenticationPipelines; // Or,
    // hashmap<UPID, Sequence> authenticationSequences;
    Also, it would be great to manage this correctly and erase entries when 
they are not relevant any more. As we discussed:
    * Store the last future from the sequence alongside here.
    * Set up a callback on future completion, the callback should check for 
equality against the current last. If equal, the entry can be erased.
    Anything I'm missing there?

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

    "within a Process' execution context"

3rdparty/libprocess/src/process.cpp (lines 2400 - 2402)

    Why are you dispatching here rather than calling directly? Note though that 
I'm not sure we should keep `schedule`.

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


3rdparty/libprocess/src/process.cpp (lines 2447 - 2455)

    The first comment here should match the first comment from the other 
blocks, no? How about:
              // Authentication succeeded.
              const Option<string>& principal = authentication.get()->principal;
              CHECK_SOME(principal); // The authentication router validates 
              // TODO(benh): Use the sender PID in order to capture
              // happens-before timing relationships for testing.
              deliver(receiver, new HttpEvent(request, promise, principal));

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

    But does this comment even belong here? Seems it belongs alongside the 
route / handler declarations.

3rdparty/libprocess/src/tests/http_tests.cpp (line 21)

    What is this for? :o

3rdparty/libprocess/src/tests/http_tests.cpp (lines 43 - 45)

    Why aren't these alphabetical?

3rdparty/libprocess/src/tests/http_tests.cpp (line 45)

    Why did you abbreviate to 'auth' here? It makes it ambiguous as to whether 
you're referring to authentication or authorization.

3rdparty/libprocess/src/tests/http_tests.cpp (lines 1205 - 1223)

    Can you instead just add another handler to the existing HttpProcess above, 
rather than introducing another Process here?
    There is already a handler for 'auth', so we can add one called 
'/authenticated' and document why '/auth' is still there (looks like it is 
going to be obviated by this?).

3rdparty/libprocess/src/tests/http_tests.cpp (line 1235)

    Please add the `()` when you use `new`

- Ben Mahler

On Dec. 8, 2015, 2:38 p.m., Alexander Rojas wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38000/
> -----------------------------------------------------------
> (Updated Dec. 8, 2015, 2:38 p.m.)
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> Bugs: MESOS-3233
>     https://issues.apache.org/jira/browse/MESOS-3233
> Repository: mesos
> Description
> -------
> Adds functions which allow libprocess users to register HTTP authenticators.
> Overloads `ProcesBase::route()` to allow for registering of authenticating 
> endpoints.
> Includes tests.
> Diffs
> -----
>   3rdparty/libprocess/include/process/event.hpp 
> a03824c061c4a0eb865b163999a763635e56744c 
>   3rdparty/libprocess/include/process/process.hpp 
> 81c094414d4d5ac5eb593df2a6d14aaacb19a826 
>   3rdparty/libprocess/src/authentication_router.hpp 
> 5777deafd7420324627802a7ab9da9aaa2b46825 
>   3rdparty/libprocess/src/process.cpp 
> e93709d3bb4ac588457bb9331fc05ec5ab539f6d 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 2de75ca1c7e224c36b534c368e7379dc158aa5bb 
> Diff: https://reviews.apache.org/r/38000/diff/
> Testing
> -------
> make check
> Thanks,
> Alexander Rojas

Reply via email to