----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52295/#review156275 -----------------------------------------------------------
Fix it, then Ship it! src/tests/hierarchical_allocator_tests.cpp (line 3532) <https://reviews.apache.org/r/52295/#comment226474> s/HierarchicalAllocatorCommonRoleQuotaTest/HierarchicalAllocatorTesWithParam/ Yeah it's hard to pick a concise yet descriptive name about the role and the quota, etc. so just leave it to the comments to explain it? src/tests/hierarchical_allocator_tests.cpp (lines 3537 - 3538) <https://reviews.apache.org/r/52295/#comment226522> Add a TODO above: ``` Move over more allocator tests that make sense to run both when the role is quota'ed and not. ``` src/tests/hierarchical_allocator_tests.cpp (line 3540) <https://reviews.apache.org/r/52295/#comment226475> s/MultiFrameworks/QuotaSwitch/ as in, the Bool() is a switch to turn quota on and off? src/tests/hierarchical_allocator_tests.cpp (line 3542) <https://reviews.apache.org/r/52295/#comment226468> s/Values(false, true)/Bool()/ src/tests/hierarchical_allocator_tests.cpp (line 3546) <https://reviews.apache.org/r/52295/#comment226486> s/it is/they are/ src/tests/hierarchical_allocator_tests.cpp (lines 3547 - 3548) <https://reviews.apache.org/r/52295/#comment226469> The correct indentation would be: ``` TEST_P(HierarchicalAllocatorTesWithParam, AllocateSharedResources) { } ``` "MultiFrameworks" doesn't feel like it's serving a qualifying purpose here (we don't have another test for a single framework). I'd say we just use the shorter test name "AllocateSharedResources". src/tests/hierarchical_allocator_tests.cpp (line 3595) <https://reviews.apache.org/r/52295/#comment226484> s/update/updated/ a better name? src/tests/hierarchical_allocator_tests.cpp (lines 3609 - 3611) <https://reviews.apache.org/r/52295/#comment226487> Not clear to me what this means... How about ``` Note that the volume is not recovered as it's used by the task (but it's still offerable because it's shared). ``` src/tests/master_validation_tests.cpp (line 696) <https://reviews.apache.org/r/52295/#comment226490> No need for this variable, just use `sharedDisk` below? src/tests/master_validation_tests.cpp (line 702) <https://reviews.apache.org/r/52295/#comment226492> s/""/"sleep 1000"/ src/tests/master_validation_tests.cpp (line 710) <https://reviews.apache.org/r/52295/#comment226494> You should be able to do: ``` pendingTasks[frameworkId1] = {{task.task_id(), task}}; ``` and thus don't need the single use variable `taskMap`. src/tests/master_validation_tests.cpp (line 718) <https://reviews.apache.org/r/52295/#comment226491> Remove a redundant space. src/tests/persistent_volume_tests.cpp (line 1199) <https://reviews.apache.org/r/52295/#comment226495> One space before `//`. src/tests/persistent_volume_tests.cpp (line 1203) <https://reviews.apache.org/r/52295/#comment226499> This line no longer relevant? src/tests/sorter_tests.cpp (line 580) <https://reviews.apache.org/r/52295/#comment226508> s/likely hood/likelyhood/ src/tests/sorter_tests.cpp (line 706) <https://reviews.apache.org/r/52295/#comment226507> No need to wrap `sharedDisk` with `Resources()`? src/tests/sorter_tests.cpp (line 732) <https://reviews.apache.org/r/52295/#comment226500> The meaning of `totalResources` here is not very clear to me: if should have disk of 1000 but here it's 900. Later the `shareDisk` is added to sorter and not to this variable. Just simply: ``` sorter.add( slaveId, Resources::parse("cpus:100;mem:100;disk(role1):900").get()); ``` ? src/tests/sorter_tests.cpp (line 736) <https://reviews.apache.org/r/52295/#comment226501> s/origTotalScalarQuantity/quantity1/ ? We are applying a sequence of operatoins to the sorter. Names like quantity1, quantity2 and quantity3 are as clear as these long names? src/tests/sorter_tests.cpp (line 738) <https://reviews.apache.org/r/52295/#comment226502> No need to wrap it with `Resources()`? Here and elsewhere. src/tests/sorter_tests.cpp (line 741) <https://reviews.apache.org/r/52295/#comment226504> Not just "NE", we expect that ``` EXPECT_EQ(Resources::parse("disk(role1):100").get(), quantity2 - quantity1); ``` right? src/tests/sorter_tests.cpp (line 749) <https://reviews.apache.org/r/52295/#comment226505> "it doesn't get removed from the sorter" I am being pedantic but the copy does get removed, just not the quantity. How about ``` // The quantity of the shared disk is removed only when the last copy is removed. ``` src/tests/sorter_tests.cpp (line 754) <https://reviews.apache.org/r/52295/#comment226506> Kill this? `sorter.remove(slaveId, Resources(sharedDisk));` is self-explanatory and the comment doesn't add additional info. - Jiang Yan Xu On Nov. 11, 2016, 10:56 a.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52295/ > ----------------------------------------------------------- > > (Updated Nov. 11, 2016, 10:56 a.m.) > > > Review request for mesos and Jiang Yan Xu. > > > Bugs: MESOS-6185 > https://issues.apache.org/jira/browse/MESOS-6185 > > > Repository: mesos > > > Description > ------- > > Tests include the following: > - Allocator tests when dealing with multiple frameworks of the same > role using the same shared volume at the same time. > - Validity of DESTROY shared volume when there are pending tasks > that have been assigned that shared volume. > - Handlng of master failover. > - Sorter tests covering shared resources. > > > Diffs > ----- > > src/tests/hierarchical_allocator_tests.cpp > edd5cea8996d7c616cf9428f0f1c05d70c37c307 > src/tests/master_validation_tests.cpp > 7f7bb675b51370036be8edb22ab6cb5f8f913146 > src/tests/persistent_volume_tests.cpp > 8651b2c5455089041f16d091c90a7e0d948191b8 > src/tests/sorter_tests.cpp 9f48abe65c011acfaf79f3d0d79f1d032fd6a4af > > Diff: https://reviews.apache.org/r/52295/diff/ > > > Testing > ------- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
