----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67616/#review204953 -----------------------------------------------------------
src/slave/constants.hpp Lines 89 (patched) <https://reviews.apache.org/r/67616/#comment287790> We should add a comment saying how we got to this magic number. Something like: `This value is set to AUFS' per-path-component limit`. We should check whether AUFS' limit is inclusive or exclusive, i.e., whether it the check should be: `containerId.length() < MAX_NESTED_CONTAINER_ID_LENGTH` or ``containerId.length() <= MAX_NESTED_CONTAINER_ID_LENGTH` src/slave/validation.cpp Lines 18 (patched) <https://reviews.apache.org/r/67616/#comment287791> We sort includes from the same component lexicographically, so this include should go before line #17. src/slave/validation.cpp Lines 75-77 (patched) <https://reviews.apache.org/r/67616/#comment287814> Is there any reason why we shouldn't perform this validation for containers without a parent container? I think we should move this block outside the `containerId.has_parent()` check. src/slave/validation.cpp Lines 76 (patched) <https://reviews.apache.org/r/67616/#comment287812> I believe that this is a user-facing error (it shows up in logs and might even be sent in an HTTP response), so it needs to be a bit friendlier. I suggest something like: ``` "'ContainerID.value' exceeds " + stringify(MAX_NESTED_CONTAINER_ID_LENGTH) +" characters". ``` src/tests/slave_validation_tests.cpp Lines 96 (patched) <https://reviews.apache.org/r/67616/#comment287813> s/id/ID/ - Gastón Kleiman On June 18, 2018, 3:56 p.m., wei xiao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67616/ > ----------------------------------------------------------- > > (Updated June 18, 2018, 3:56 p.m.) > > > Review request for mesos, Gilbert Song and Jie Yu. > > > Bugs: MESOS-7168 > https://issues.apache.org/jira/browse/MESOS-7168 > > > Repository: mesos > > > Description > ------- > > Added the limitation of the container id length. > > > Diffs > ----- > > src/slave/constants.hpp b97daf3d2eb04b796de5283d9adb0f515ca69f8c > src/slave/validation.cpp 09f1fc702a1b4550c04bc9c99d5ebd17974ebbb1 > src/tests/slave_validation_tests.cpp > d8bc142dd707f0888c29bf070135d5d0083ef421 > > > Diff: https://reviews.apache.org/r/67616/diff/2/ > > > Testing > ------- > > > Thanks, > > wei xiao > >
