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. Let me know if I miss any other
cases.
2. Changes in existing tests cover
`LeastBytesUsedStorageLocationSelectorStrategy` as this is the default strategy.
> including when multiple threads are calling it.
This is something I missed. Is
`SegmentLoaderLocalCacheManagerConcurrencyTest` a write place to add ?
----------------------------------------------------------------
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]
