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 44946: Add utility functions to manipulate XFS project quotas.
Date Wed, 23 Mar 2016 17:39:18 GMT

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



My thoughts about the discussions around: 

1) Using `path` vs. `device` in quota APIs.
It's a bit confusing but since `xfs_quota` also using `project paths` instead of devices,
I think it's fine as long as we clearly document what it is.


2) passing `isdir` in the public APIs: the APIs here are for processing each file/dir which
is driven by the `scanDirectory` method in the isolator. If the APIs operate at a higher-level
and take directories as arguments and move the directory hierarchy traversal logic over, then
the API is very clean-cut and `isdir` is hidden in private functions.

Then why move them out at all? IMO it's still worthwhile as we gain testability and clear
separate of concerns between serving the isolator API (recovery, ID management, etc.) and
the XFS building blocks (readability and maintainability), which can be easily understood.

I realize that the APIs are not *generic* enough for all uses of XFS quota and I am OK with
`utils.hpp|cpp` being under `isolators` for now but IMHO all files under `src/linux` are not
generic (as they should be) so I wouldn't worry about them being under linux/xfs. (Not sure
if we have other uses of XFS features in the future that share anything in common).


Sample APIs:
```
namespace mesos {
namespace internal {
namespace xfs {

// Get the quota associated with the project ID on the block device that backs the 
// specified `path`.
Try<QuotaInfo> getProjectQuota(
    const std::string& path,
    prid_t projectId);


Try<Nothing> setProjectQuota(
    const std::string& path,
    prid_t projectId,
    Bytes limit);


Try<Nothing> clearProjectQuota(
    const std::string& path,
    prid_t projectId);


// Return the project ID associated with this directory itself. Note
// that we don't verify if the directory contents have different  
// project IDs.
Try<prid_t> getProjectId(const std::string& directory);


Try<Nothing> setProjectId(
    const std::string& directory,
    prid_t projectId);


Try<Nothing> clearProjectId(
    const std::string& directory);

// In cpp.
namespace internal {

Try<Nothing> setProjectId(
    const std::string& path,
    prid_t projectId,
    bool isdir = true);

...
    
} // namespace internal
} // namespace xfs
```


src/slave/containerizer/mesos/isolators/disk/xfs/utils.hpp (line 39)
<https://reviews.apache.org/r/44946/#comment187689>

    



src/slave/containerizer/mesos/isolators/disk/xfs/utils.hpp (line 69)
<https://reviews.apache.org/r/44946/#comment187665>

    Kills the line.



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (lines 17 - 19)
<https://reviews.apache.org/r/44946/#comment187666>

    So this is bug of the xfs headers?
    
    I guess it's still not clear to me why this is relevant here. Is it a bug of XFS? 
    
    It looks like textdomain() is for i18n support. I guess this has to do with path names
being non ASCII? When ENABLE_GETTEXT is definied here and no other dependencies are checked,
will XFS headers go the right thing?
    
    This might be too long of a discussion for here so I was suggesting a link where the above
question is answered (or source).



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 62)
<https://reviews.apache.org/r/44946/#comment187676>

    s/Xfs// since we are already in the xfs namespace.



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 74)
<https://reviews.apache.org/r/44946/#comment187677>

    s/Xfs// since we are already in the xfs namespace and more idiomatic to return the result.
    
    ```
    Try<struct fsxattr> getAttributes(int fd) {...}
    ```



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 88)
<https://reviews.apache.org/r/44946/#comment187678>

    When function signatures fit in one line, no need to put arguments on new lines.



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 109)
<https://reviews.apache.org/r/44946/#comment187681>

    So this is the internal helper for setting project quota. Our convention doesn't require
heplers to be inside an `internal` namespace but when there's difficulty in naming them, it's
clearer to distinguish things if we have 
    
    `xfs::setProjectQuota` and `xfs::internal::setProjectQuota`
    
    vs. 
    
    `xfs::xfsSetProjectQuota` and `xfs::setProjectQuota`



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 119)
<https://reviews.apache.org/r/44946/#comment187679>

    I know I asked this previoiusly but what's the difference between this and:
    
    ```
    fs_disk_quota_t quota;
    ```
    
    If it's the same, then the more common syntax is preferred because this syntax reads like
the 1st member of the struct has special some meaning/usage.



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 127)
<https://reviews.apache.org/r/44946/#comment187682>

    s/solf/soft/.



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 133)
<https://reviews.apache.org/r/44946/#comment187680>

    The magic number 512 is used enough times to warrant a constexpr variable IMO. Plus it's
not clear in the comments in this file that the **fixed** *basic block size* as opposed to
the **configurable** *block size* is used in these measurements.
    
    By definiting such a constant you can also clearly document this.



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (lines 295 - 297)
<https://reviews.apache.org/r/44946/#comment187706>

    ```
    Try<Nothing> status = getXfsAttributes(fd.get(), attr);
    ```



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 317)
<https://reviews.apache.org/r/44946/#comment187683>

    Kill the line.


- Jiang Yan Xu


On March 22, 2016, 4:24 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> -----------------------------------------------------------
> 
> (Updated March 22, 2016, 4:24 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
>     https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/slave/containerizer/mesos/isolators/disk/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> -------
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>


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