sashidhar commented on a change in pull request #8038: Making optimal usage of
multiple segment cache locations
URL: https://github.com/apache/incubator-druid/pull/8038#discussion_r317371552
##########
File path:
server/src/test/java/org/apache/druid/segment/loading/SegmentLoaderLocalCacheManagerTest.java
##########
@@ -372,24 +372,276 @@ public void testEmptyToFullOrder() throws Exception
}
private DataSegment dataSegmentWithInterval(String intervalStr)
+ {
+ return dataSegmentWithInterval(intervalStr, 10L);
+ }
+
+ private DataSegment dataSegmentWithInterval(String intervalStr, long size)
{
return DataSegment.builder()
- .dataSource("test_segment_loader")
- .interval(Intervals.of(intervalStr))
- .loadSpec(
- ImmutableMap.of(
- "type",
- "local",
- "path",
- "somewhere"
- )
- )
- .version("2015-05-27T03:38:35.683Z")
- .dimensions(ImmutableList.of())
- .metrics(ImmutableList.of())
- .shardSpec(NoneShardSpec.instance())
- .binaryVersion(9)
- .size(10L)
- .build();
+ .dataSource("test_segment_loader")
+ .interval(Intervals.of(intervalStr))
+ .loadSpec(
+ ImmutableMap.of(
+ "type",
+ "local",
+ "path",
+ "somewhere"
+ )
+ )
+ .version("2015-05-27T03:38:35.683Z")
+ .dimensions(ImmutableList.of())
+ .metrics(ImmutableList.of())
+ .shardSpec(NoneShardSpec.instance())
+ .binaryVersion(9)
+ .size(size)
+ .build();
+ }
+
+ @Test
+ public void testSegmentDistributionUsingRoundRobinStrategy() throws Exception
Review comment:
> The new tests here are quite duplicate and incomplete. The unit tests
should verify that the locations returned by the strategy is ordered correctly
in various situations
1. The tests do verify the locations returned by the strategy are ordered
correctly or not.
Common reusable bits of code have been extracted into methods.
In `testSegmentDistributionUsingRoundRobinStrategy` .
The below comments in the test make it clear that for round robin strategy
segment1 is downloaded into local_storage_folder, segment2 to folder2, segment3
to folder3 and that the segment4 is downloaded into local_storage_folder
asserting round robin behavior.
```
// Segment 1 should be downloaded in local_storage_folder
// Segment 2 should be downloaded in local_storage_folder2
// Segment 3 should be downloaded in local_storage_folder3
// Segment 4 should be downloaded in local_storage_folder again, asserting
round robin distribution of segments
```
One case I could add was setting a `localStorageFolder` unwritable, but this
is covered in tests `testRetrySuccessAtSecondLocation` and others. Moreover
writable or not isn't checked by the strategy.
2. `testSegmentDistributionUsingLeastBytesUsedStrategy` and changes in
existing tests cover least bytes used strategy as this is the default strategy.
Please look at below comments. The corresponding assert statements in the test
assert the least bytes used distribution of segments.
```
// Segment 1 should be downloaded in local_storage_folder, segment1 size 10L
// Segment 2 should be downloaded in local_storage_folder2, segment2 size 5L
// Segment 3 should be downloaded in local_storage_folder3, segment3 size 20L
// Now the storage locations local_storage_folder1, local_storage_folder2
and local_storage_folder3 have 10, 5 and
// 20 bytes occupied respectively. The default strategy should pick
location2 (as it has least bytes used) for the
// next segment to be downloaded asserting the least bytes used distribution
of segments.
```
Let me know if I missed any other cases or something is unclear.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
