This is an automatically generated e-mail. To reply, visit:

src/slave/containerizer/mesos/isolators/xfs/utils.hpp (line 17)

    Now that this header was moved: `s/__XFS_HPP__/__XFS_UTILS_HPP__/`

src/slave/containerizer/mesos/isolators/xfs/utils.hpp (line 39)

    s/operator ==/operator==/ as per convention.

src/slave/containerizer/mesos/isolators/xfs/utils.hpp (line 61)

    It would be great if we can have consitently named two set of methods:
    set|assign & remove|clear are basically synonymous so we can choose either 
but keep them consistent.

src/slave/containerizer/mesos/isolators/xfs/utils.hpp (line 62)

    s/path/direcotry/ and we can do a `isdir` check inside.

src/slave/containerizer/mesos/isolators/xfs/utils.hpp (line 67)

    4 space indetation & s/path/directory/.

src/slave/containerizer/mesos/isolators/xfs/utils.hpp (lines 70 - 72)

    Since we don't do recursive calls for `getProjectId`, I think we can just 
internally detect if the path is a directory without concerns about performance.
    Result<prid_t> getProjectId(const std::string& path);

src/slave/containerizer/mesos/isolators/xfs/utils.hpp (lines 75 - 83)

    These two methods don't need to appear in the header as they are in fact 
just two internal callbacks driven by the public version of their namesakes.
    We can put them in the cpp file: 
    namespace internal {
    static Try<Nothing> setProjectId(
        const string& path, 
        prid_t projectId,
        const struct stat& stat);
    static Try<Nothing> clearProjectId(
        const string& path, 
        prid_t projectId,
        const struct stat& stat)
    In fact, I think we can further simplify by removing `clearProjectId()` as 
it is really just a special case of `setProjectId()`. 
    I understand that separating them is for clarity and you go a check
    if (projectId == NON_PROJECT_ID) {
      return NonProjectError();
    in `setProjectId()` so it's less error-prone to use.
    However we can do the check in the public version of `setProjectId()` so 
the private `setProjectId()` doesn't need it do it again per file/directory.
    Without this check the private versions of `setProjectId()` and 
`clearProjectId()` are identical and we reduce code duplication by consolidate 
them into one.
    See my comments on `xfsSetProjectQuota()` for the internal namespace 
    Also I see that in the tests you have directly used `setProjectId(path)` on 
a file directly so moving this into cpp would force you to change it. I'll 
comment on that test directly.

src/slave/containerizer/mesos/isolators/xfs/utils.cpp (lines 19 - 20)

    I think the last sentence is too specific to CentOS and the first two 
sentences have already provided sufficient infomation, i.e., "xfsprogs versions 
earlier than 4.5", so we can remove the last sentence.

src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 59)

    Nit: 512u.

src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 60)

    Add an empty line.

src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 67)

    This is capitalized so it works like a class?
    Can we call it `nonProjectError()`?

src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 69)


src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 131)

    It would be more idiomatic to put this in a 
    namespace internal {
      static Try<Nothing> setProjectQuota(
          const string& path,
          prid_t projectId,
          Bytes limit)
    So when in your public `setProjectQuota()` it'll be calling 
`internal::setProjectQuota()`, which is clear that a wrapper is calling some 
internal helper, as compared to calling `xfsSetProjectQuota()`. (Because this 
entire file is for XFS so a `xfs` prefix doesn't disambiguate as effectively. 
The same applies to using "assign vs. set" to disambiguate.

src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 172)

    Pull this down to right above `info.limit = ...`.

src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 222)

    Align it with the open paren above. We don't use 4 space indentation if 
this is not a list of function arguments.

src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 245)

    As suggested above, we can detect `isdir` inside the method.

src/slave/containerizer/mesos/isolators/xfs/utils.cpp (lines 271 - 273)

    Move this to the public version of `setProjectId()`

src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 302)

    As suggested above, kill this method in favor of reusing 

src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 302)


src/slave/containerizer/mesos/isolators/xfs/utils.cpp (lines 333 - 334)

    There is a lot of code duplication in the following two methods.
    I think it should be reasonable to stick to the previous approach to have a 
common method:
    Try<Nothing> traverse(
        const string& directory,
        std::function<Try<Nothing>(const string&, const struct stat&)> callback)
    For this review let's keep the helper in this file but it's nevertheless 
more elegant and mantainable.

src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 335)

    rmdir.hpp has this:
        // NOTE: `fts_open` will not always return `NULL` if the path does not
        // exist. We manually induce an error here to indicate that we can't 
        // a directory that does not exist.
        if (!os::exists(directory)) {
          errno = ENOENT;
          return ErrnoError();
    I think we can do something similar. Plus, will fts_open this fail with 
ENOTDIR if it's not a dir?
    I think we do a more explicitly check:
    if (!stat::isdir(directory)) {
      return Error("'" + directory + "' is not a directory");

src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 336)

    We often use a trailing understore: `char* directory_[]`. 
(s/path/directory/ is due to the suggestion on generalizing the method)
    Also, no padding space in `{}`.
    char* directory_[] = {const_cast<char*>(path.c_str()), nullptr};

src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 339)

    You've been using nullptr so s/NULL/pullptr/ for consistency.

src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 344)

    One more space to the right to align it with the open paren. (Since this is 
not a function call).

src/slave/containerizer/mesos/isolators/xfs/utils.cpp (lines 346 - 347)

    Here we can `callback(node->fts_path, *node->fts_statp);`.

src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 357)

    Here we could have said
    "Failed to fully process the directory tree '" + path + "'"
    but we are going to prepend caller specific message anyway so I think 
    Error error = ErrnoError();
    is sufficient.

src/slave/containerizer/mesos/isolators/xfs/utils.cpp (lines 366 - 368)

    In this method we can just validate the projectId first and then 

- Jiang Yan Xu

On March 30, 2016, 3:18 p.m., James Peach wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> -----------------------------------------------------------
> (Updated March 30, 2016, 3:18 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 f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/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