mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexander Rojas" <alexan...@mesosphere.io>
Subject Re: Review Request 37999: Implemented http::AuthenticatorManager
Date Wed, 04 Nov 2015 11:25:51 GMT


> On Oct. 21, 2015, 1:03 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/include/process/authenticator.hpp, line 52
> > <https://reviews.apache.org/r/37999/diff/9/?file=1102345#file1102345line52>
> >
> >     Duplicate text.

I used a separate line because Doxygen uses the first line as a brief summary (It is also
in our [style guide](https://github.com/apache/mesos/blob/c3940cd4da29eb6539096c6ec6dcab1a70993c42/docs/doxygen-style-guide.md#source-code-documentation-syntax)).
However, given that I haven't seen the rendered result. I will remove this line.


> On Oct. 21, 2015, 1:03 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/include/process/authenticator.hpp, line 107
> > <https://reviews.apache.org/r/37999/diff/9/?file=1102345#file1102345line107>
> >
> >     s/are/is (contents is singular)
> >     
> >     s/a HTTP/an HTTP 
> >     
> >     (Please check all other places. If "a" is followed by "H" in an abbreviation,
which is therefore pronounced "aytsh", this constitutes an "a" followed by a vowel sound,
so it becomes "an)

Sorry, _contents_ is [plural](http://english.stackexchange.com/questions/13556/content-or-contents).
Though there are different meanings of the word _content_.


> On Oct. 21, 2015, 1:03 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/src/process.cpp, line 793
> > <https://reviews.apache.org/r/37999/diff/9/?file=1102348#file1102348line793>
> >
> >     Suggestion: break this up into two calls.
> >     
> >     removeAllAuthenticators()
> >     removeAuthenticator(<param>)

I will let you decide this one with till, because he suggested the exact opposite (which has
also been suggested by other reviewers), in his Sept. 10, 2015, 8:06 a.m. review (see above).


- Alexander


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/#review103383
-----------------------------------------------------------


On Nov. 4, 2015, 12:25 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> -----------------------------------------------------------
> 
> (Updated Nov. 4, 2015, 12:25 p.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
> -------
> 
> Introduces the authenticator manager, which is a class which handles the actual authentication
procedure during the execution of `ProcessManager::handle()` and it also takes care of the
life cycle of instances of http::Authenticator.
> 
> No tests are added at this point since no public API is generated, the goal of this patch
is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests
proper come in a latter patch.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae

>   3rdparty/libprocess/include/process/http.hpp 90c9be122ee0c402b806d70fc818e3c03b15101a

>   3rdparty/libprocess/src/process.cpp a94712b9ac3b60fb047b3a5a4d84a56fa4d02313 
> 
> Diff: https://reviews.apache.org/r/37999/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


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