mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 57190: Updated agent for hierarchical roles.
Date Fri, 31 Mar 2017 12:54:19 GMT


> On March 28, 2017, 2:35 a.m., Benjamin Mahler wrote:
> > src/slave/paths.cpp
> > Line 451 (original), 451-452 (patched)
> > <https://reviews.apache.org/r/57190/diff/4/?file=1660519#file1660519line451>
> >
> >     It would be great to expand on why we do this. Also, on how we arrived on using
spaces (i.e. as it pertains to the allowed characters within roles, whether space is valid
on multiple filesystems / OS's, etc).

I added some details here. Let me know if you believe it doesn't highlight a critical aspect
enough.


> On March 28, 2017, 2:35 a.m., Benjamin Mahler wrote:
> > src/tests/role_tests.cpp
> > Lines 788-790 (patched)
> > <https://reviews.apache.org/r/57190/diff/4/?file=1660520#file1660520line788>
> >
> >     This seems a little obscure to me, how about clarifying the problem (i.e. related
to the directories and the use of slash, which is why we have the workaround with space characters).
That should give enough context for someone to understand why we would be testing this.

I expanded the comment.


> On March 28, 2017, 2:35 a.m., Benjamin Mahler wrote:
> > src/tests/role_tests.cpp
> > Lines 946-949 (patched)
> > <https://reviews.apache.org/r/57190/diff/4/?file=1660520#file1660520line946>
> >
> >     Ah here is what I was looking for, this comment is helpful! Could we have a
comment like this at the top of the test to give the context?

Added a somewhat condensed version in the test doc block.


- Benjamin


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


On March 31, 2017, 2:54 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57190/
> -----------------------------------------------------------
> 
> (Updated March 31, 2017, 2:54 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-7047
>     https://issues.apache.org/jira/browse/MESOS-7047
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This commit adjusts the way persistent volumes are stored on the
> agent. Instead of interpreting the role of the volume as a literal
> path, we replace `/` with ` ` when creating the path. This prevents
> that subdirectories are created for volumes with hierarchical roles.
> Directly interpreting the role as a path is undesirable as it can lead
> to volumes overlapping (e.g., a volume with role `a/b/c/d` and id `id`
> would be visible as `id` in a volume with role `a/b/c` and id `d`).
> 
> 
> Diffs
> -----
> 
>   src/slave/paths.cpp ef22ee167f16030f02d28c8e6bab6c2ca4812d8f 
>   src/tests/role_tests.cpp 0433c0599eac5f4648bc0dfe3a0fa8d5f7a836ca 
> 
> 
> Diff: https://reviews.apache.org/r/57190/diff/5/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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