mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 54449: Check quotas are enabled in the XFS disk isolator.
Date Thu, 02 Mar 2017 10:49:19 GMT

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


Fix it, then Ship it!




Sorry this was mostly done but got overlooked due to holidays...

We have discussed the unit test before and it's probably fine if we punt on that (I still
think a negative test case is useful) but could you update the "Testing Done" section with
a manual test negative case result? The negative case is the whole point of this patch after
all. :)


src/slave/containerizer/mesos/isolators/xfs/utils.cpp
Lines 408-412 (patched)
<https://reviews.apache.org/r/54449/#comment239583>

    This is very thorough, thanks. :) 
    
    I originally just meant that we could comment on what each `0`  means on their own line,
much like (this)[https://github.com/apache/mesos/blob/cc15db4b10af343d7aefc838bb626f4579289375/src/tests/containerizer/xfs_quota_tests.cpp#L128]
and perhaps copy some snippets from the external doc for the sake of "explaining obscure APIs/terms".
    
    Would it make sense to s/we are getting/it is for/? The difference to me is whether the
API has to be used this way or we are using the API in this particular way. Sounds like the
former is true?



src/slave/containerizer/mesos/isolators/xfs/utils.cpp
Lines 412 (patched)
<https://reviews.apache.org/r/54449/#comment239584>

    s/identity/identity (e.g., projectId)/?
    
    In this file we have been using `projectId` throughout and I as a reader became more familiar
with that term than `identity`.


- Jiang Yan Xu


On March 1, 2017, 10:50 a.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54449/
> -----------------------------------------------------------
> 
> (Updated March 1, 2017, 10:50 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6732
>     https://issues.apache.org/jira/browse/MESOS-6732
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The XFS disk isolator checks that the filesystem is XFS, but doesn't
> check whether project quotas are actually enabled. This means that
> an invalid configuration will start but will always fail when tasks
> are launched.
> 
> Add a check to test whether project quotas are enabled on the work
> directory and fail hard if they are not.
> 
> 
> Diffs
> -----
> 
>   configure.ac 7a18c89854c1f42bec09f067579b72114fb8ec1d 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp dd4df86bf90bfa9cbf4664d89274cf3c64c2e374

>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp 7602fe3b6ab069db643397418732e773d0417f8a

>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp b9d8e7dc999ba3064bee7105eff0f9553d825df8

> 
> 
> Diff: https://reviews.apache.org/r/54449/diff/3/
> 
> 
> Testing
> -------
> 
> Make check on Fedora 25. Manual test on F25 with mesos-execute.
> 
> 
> Thanks,
> 
> James Peach
> 
>


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