-----------------------------------------------------------
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
> 
>

Reply via email to