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