mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marco Massenzio" <ma...@mesosphere.io>
Subject Re: Review Request 38627: Adds an overload of ModuleManager::create() allowing overriding parameters programatically
Date Mon, 12 Oct 2015 12:54:08 GMT

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


Would it be possible to add a few unit tests, also to show usage patterns? especially given
the absence of any documentation, it's kinda difficult to figure out how is this "intended
to work" and, without tests, whether it works at all.


src/module/manager.hpp (line 102)
<https://reviews.apache.org/r/38627/#comment159819>

    would it be possible to have proper, full javadoc for this method (as well as the other
one)?
    these will be used by external module developers, so having them well-documented would
be really great.



src/tests/module.hpp (lines 76 - 77)
<https://reviews.apache.org/r/38627/#comment159821>

    same comment here - it would be great to have fully-documented, properly-formatted javadoc
here, also explaining what could go into `parameters` how will this affect creating the module,
etc.
    
    while this may all appear obvious to you, it may not be so to external developers using
this code to launch/test modules.
    
    thanks!



src/tests/module.hpp (line 80)
<https://reviews.apache.org/r/38627/#comment159823>

    (I understand this may not be your choice, but...)
    
    I find `N` as an identifier is a poor choice because (a) it's entirely non-obvious *what*
is it and (b) I'd expect it anyway to be an `int` of some sort.
    
    Could you please consider renaming it to be something more expressive?
    
    thanks!


- Marco Massenzio


On Oct. 2, 2015, 8:11 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38627/
> -----------------------------------------------------------
> 
> (Updated Oct. 2, 2015, 8:11 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Niklas Nielsen, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3232
>     https://issues.apache.org/jira/browse/MESOS-3232
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Allows developers to provide their own parameters when loading modules instead of using
the ones provided by the user when loading Mesos. This helps to deal with default modules
(those used when the user doesn't provide any), and for testing of the modules.
> 
> 
> Diffs
> -----
> 
>   src/module/manager.hpp 302eb409fb8ef53b9cef8d2ecbe7b7f452b095ef 
>   src/tests/module.hpp 0820978441aede18dae6d1701433bff705b8c3c2 
> 
> Diff: https://reviews.apache.org/r/38627/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


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