> On March 8, 2016, 11:20 p.m., Adam B wrote: > > src/tests/hierarchical_allocator_tests.cpp, line 233 > > <https://reviews.apache.org/r/43824/diff/6-7/?file=1282878#file1282878line233> > > > > `getAllocations` is too general a name for what this function actually > > does. More like `handleTwoAllocationsAndRecoverResources`. Any method that > > starts with `get` should return something, preferably whatever comes after > > `get` (i.e. `Allocations`). > > If you parameterize the number of iterations of the loop, add a > > boolean/enum for whether to recover the resources, and return > > `frameworkAllocations`, then you could call it `getAllocations`, and use it > > for the 3-iteration loop at the end of the test. > > That said, only two reuses may not be worth pulling it out (although 3 > > may be). > > I give you 3 options (your choice): > > 1. Rename the method appropriately (may require code changes so it's > > not as ugly as `handleTwoAllocationsAndRecoverResources`). > > 2. Inline the method back into the test in both places. > > 3. Generalize the method and reuse it for the 3rd instance.
Thanks very much for your detailed description. - Yongqiao ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43824/#review122622 ----------------------------------------------------------- On March 8, 2016, 12:10 p.m., Yongqiao Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43824/ > ----------------------------------------------------------- > > (Updated March 8, 2016, 12:10 p.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 > 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 > > Diff: https://reviews.apache.org/r/43824/diff/ > > > Testing > ------- > > make && make check successfully. > > > Thanks, > > Yongqiao Wang > >
