mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Niklas Nielsen" <...@qni.dk>
Subject Re: Review Request 26071: Sample isolator module.
Date Thu, 16 Oct 2014 22:24:42 GMT

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



src/Makefile.am
<https://reviews.apache.org/r/26071/#comment97452>

    'Library containing test CPU and memory isolator modules'?



src/examples/test_isolator_module.cpp
<https://reviews.apache.org/r/26071/#comment97481>

    Let's use modules@mesos.apache.org (it is working) instead - We don't want to generate
modules traffic on the regular dev list.



src/examples/test_isolator_module.cpp
<https://reviews.apache.org/r/26071/#comment97480>

    Why 'Test' and not Test?



src/slave/containerizer/isolator.hpp
<https://reviews.apache.org/r/26071/#comment97474>

    Reinsert newline please



src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/26071/#comment97473>

    Don't know if we have any specific wrapping rules wrt very long templates. This looks
off to me - but let's look into that.



src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/26071/#comment97472>

    same here



src/tests/main.cpp
<https://reviews.apache.org/r/26071/#comment97477>

    Mind expanding on, that the two modules lists are merged or that they compliment each
other?



src/tests/module.hpp
<https://reviews.apache.org/r/26071/#comment97467>

    Mind adding a comment on why you neem this enum?



src/tests/module.hpp
<https://reviews.apache.org/r/26071/#comment97468>

    You could add a comment here too explaining how it is used with type_param'ed tests



src/tests/module.cpp
<https://reviews.apache.org/r/26071/#comment97479>

    Don't you need an include for hashmap (rather than relying on it being included in one
of the headers?)
    
    Same for path::join



src/tests/module.cpp
<https://reviews.apache.org/r/26071/#comment97459>

    Mind adding a comment on how future test isolators have to be added? :-)



src/tests/module.cpp
<https://reviews.apache.org/r/26071/#comment97464>

    I have a hard time understanding how this comment ties into the functionality of initModules()
Can you expand/clarify? Or does it belong to moduleNames() instead?



src/tests/module.cpp
<https://reviews.apache.org/r/26071/#comment97460>

    Please expand 'm' to a full word



src/tests/module.cpp
<https://reviews.apache.org/r/26071/#comment97462>

    Can you expand a bit on what went wrong? The error message is a bit too general / doesn't
add much context.



src/tests/module.cpp
<https://reviews.apache.org/r/26071/#comment97478>

    Mind distinguish type and kind? Now, you fact called it an ID :-)



src/tests/module.cpp
<https://reviews.apache.org/r/26071/#comment97465>

    Can you include the module id value in the error message? Makes it a bit easier to debug.


- Niklas Nielsen


On Oct. 16, 2014, 12:20 p.m., Kapil Arya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26071/
> -----------------------------------------------------------
> 
> (Updated Oct. 16, 2014, 12:20 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Ian Downes, and Niklas Nielsen.
> 
> 
> Bugs: MESOS-1384
>     https://issues.apache.org/jira/browse/MESOS-1384
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Created CPU and Memory isolation modules based on Posix CPU/Memory Isolators.  These
modules are also hooked up to the relevant Isolator tests using the typed test mechanism.
 It also paves the way for future integration with custom modules specified on the command
line.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am d503c8df73cda15a9d59254e8265e4a5d0e003a4 
>   src/examples/test_isolator_module.cpp PRE-CREATION 
>   src/module/isolator.hpp PRE-CREATION 
>   src/module/manager.hpp 797728a8c8e88dd1a13142a355cbe0b1491fb7a2 
>   src/module/manager.cpp 8cc79956a8d3d7cb27016889ec59d138a0b58e45 
>   src/slave/containerizer/isolator.hpp e52e8b15c740c62ef64b49897d3d6ae5179d4719 
>   src/tests/isolator_tests.cpp c38f87632cb6984543cb3767dbd656cde7459610 
>   src/tests/main.cpp 90a7ddcb5695d4c9a7760e18285548d99e7bcfa8 
>   src/tests/module.hpp PRE-CREATION 
>   src/tests/module.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/26071/diff/
> 
> 
> Testing
> -------
> 
> make check with an  Isolator module test.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>


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