----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44948/#review124486 -----------------------------------------------------------
Overall, looks good to me! src/slave/containerizer/mesos/isolators/disk/xfs.hpp (line 87) <https://reviews.apache.org/r/44948/#comment187073> Can we make this 'const' as well and only allow setting it in the constructor? src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 17) <https://reviews.apache.org/r/44948/#comment187065> Can you put this below stout headers. I think we're not following google style here. Please check out other files. src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 172 - 173) <https://reviews.apache.org/r/44948/#comment187080> Any reason this is not in xfs.hpp|cpp? src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 290 - 291) <https://reviews.apache.org/r/44948/#comment187066> Let's try not to use CHECK here. Can you do checks in XfsDiskIsolatorProcess::create and return Error instead. src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 295) <https://reviews.apache.org/r/44948/#comment187069> We rarely use more than 1 assignment in one statement. Can you split them so that it's more explicit. src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 300) <https://reviews.apache.org/r/44948/#comment187070> Ditto. Let's return Error in 'create' method. src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 302 - 303) <https://reviews.apache.org/r/44948/#comment187067> We typically store the 'flags' directory in case yo u need other parameters in the future. src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 320) <https://reviews.apache.org/r/44948/#comment187075> I am wondering if it's possible to distinguish the Error case from the case where there's no project ID assigned to the directory. Can we make xfs::getProjectId(...) return a Result? src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 364) <https://reviews.apache.org/r/44948/#comment187074> I would prefer to return an Error in this case because otherwise, you don't know if some terminated executor's sandboxes have project ID assigned or not. ``` if (sandboxes.isError()) { return Error("..."); } foreach (...) ``` Nits: please insert a space after 'foreach'. src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 394) <https://reviews.apache.org/r/44948/#comment187076> Can you have a LOG(ERROR) here? src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 416) <https://reviews.apache.org/r/44948/#comment187077> Instead of using 0 to denote the exhausted case, can we change the interface of nextProjectId to return an Option<prid_t> (None() means exhausted). We usually prefer more explicit way. src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 544 - 551) <https://reviews.apache.org/r/44948/#comment187079> ``` if (cleanup.isError()) { totalProjectIds -= info->projectId; freeProjectIds -= info->projectid; } else { returnProjectId(info->projectId); } ``` - Jie Yu On March 21, 2016, 2 a.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44948/ > ----------------------------------------------------------- > > (Updated March 21, 2016, 2 a.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 > ------- > > Track sandbox directory usage by dynamically assigning XFS project > quotas. We track a range of XFS project IDs, assigning a project ID > and a project quota to each sandbox as it is created. When the task > reaches the quota, writes will fail with EDQUOT, and the task will have > an opportunity to handle that. > > Quotas are not applied to volume resources since the isolator interface > has no insight into the volume lifecycle. Thus it is not currently > possible to accurately assign and reclaim project IDs. > > If LOW is the lower bound of the project ID range and HIGH is the upper > bound, you can show the currently allocated project quotas using the > xfs_quota command: > > $ xfs_quota -x -c "report -a -n -L LOW -U HIGH" > > To show the project ID assigned to the file PATH, use the xfs_io command: > > $ xfs_io -r -c stat PATH > > > Diffs > ----- > > src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf > src/slave/containerizer/mesos/containerizer.cpp > ee7a265975323ca891114a286357c8e42901560c > src/slave/containerizer/mesos/isolators/disk/xfs.hpp PRE-CREATION > src/slave/containerizer/mesos/isolators/disk/xfs.cpp PRE-CREATION > src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 > src/slave/flags.cpp b77afa956834bb5b1f85301d7a5f386ab9da41e3 > > Diff: https://reviews.apache.org/r/44948/diff/ > > > Testing > ------- > > Make check. Manual testing. Tests in subsequent patches. > > > Thanks, > > James Peach > >
