> On March 3, 2016, 9:03 a.m., Alexander Rukletsov wrote: > > src/tests/hierarchical_allocator_tests.cpp, lines 2607-2609 > > <https://reviews.apache.org/r/43824/diff/2/?file=1275001#file1275001line2607> > > > > I'm an ESL, but having both "per weight" and "by weight" sounds a bit > > strange. Maybe Adam can help wiht finding the right preposition.
Thanks Alex. @Adam, I am also an ESL. What is your suggestion for this? > On March 3, 2016, 9:03 a.m., Alexander Rukletsov wrote: > > src/tests/hierarchical_allocator_tests.cpp, lines 2635-2640 > > <https://reviews.apache.org/r/43824/diff/2/?file=1275001#file1275001line2635> > > > > Does it fit one line? I know we are a bit inconsistent about it, but > > let's prefer oneliners where it doesn't impact the readability. Put those parameters into one line will exceed 80 chars, then `git commit` will failed due to the default check. I agree with your suggestion, but I think we should have a discussion in community to enhance the related check rules. > On March 3, 2016, 9:03 a.m., Alexander Rukletsov wrote: > > src/tests/hierarchical_allocator_tests.cpp, lines 2652-2654 > > <https://reviews.apache.org/r/43824/diff/2/?file=1275001#file1275001line2652> > > > > How about > > > > // Framework2 registers with 'role2' which also uses the default > > weight. It > > // will not get any offers because all resources are offered to > > `framework1`. I remember I wrote this comment like you before. The current comment is changed with Adam's comments. @Adam, any further comments for this? > On March 3, 2016, 9:03 a.m., Alexander Rukletsov wrote: > > src/tests/hierarchical_allocator_tests.cpp, line 2850 > > <https://reviews.apache.org/r/43824/diff/2/?file=1275001#file1275001line2850> > > > > Why do we need to `resume` here? It does not needed, I have removed this line. > On March 3, 2016, 9:03 a.m., Alexander Rukletsov wrote: > > src/tests/hierarchical_allocator_tests.cpp, lines 2736-2738 > > <https://reviews.apache.org/r/43824/diff/2/?file=1275001#file1275001line2736> > > > > I think these lines are good candidate to go under the `{}` together > > with the checks. This way you can avoid numeral suffixes and have a 1:1 > > relation between `{}`-blocks and test cases. > > > > Does it make sense? Update weights should be a completely different logic with the above check, so it would not be proper to put them together. > On March 3, 2016, 9:03 a.m., Alexander Rukletsov wrote: > > src/tests/hierarchical_allocator_tests.cpp, lines 2790-2792 > > <https://reviews.apache.org/r/43824/diff/2/?file=1275001#file1275001line2790> > > > > See my comment above about how to avoid numeral suffixes. Same comments as above. - Yongqiao ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43824/#review121642 ----------------------------------------------------------- On March 1, 2016, 6:42 a.m., Yongqiao Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43824/ > ----------------------------------------------------------- > > (Updated March 1, 2016, 6:42 a.m.) > > > Review request for mesos, Adam B and Alexander Rukletsov. > > > Bugs: MESOS-4200 > https://issues.apache.org/jira/browse/MESOS-4200 > > > Repository: mesos > > > Description > ------- > > Addressed comments of 41672. > > > Diffs > ----- > > src/tests/hierarchical_allocator_tests.cpp > 5f771f02db9bd098f3cd36730cd84bf2f5e87a33 > > Diff: https://reviews.apache.org/r/43824/diff/ > > > Testing > ------- > > make && make check successfully. > > > Thanks, > > Yongqiao Wang > >
