You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/09/07 11:30:46 UTC
[GitHub] [druid] FrankChen021 opened a new pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
FrankChen021 opened a new pull request #10363:
URL: https://github.com/apache/druid/pull/10363
<!-- Thanks for trying to help us make Apache Druid be the best it can be! Please fill out as much of the following information as is possible (where relevant, and remove it when irrelevant) to help make the intention and scope of this PR clear in order to ease review. -->
This PR fixes #10348 , which is caused by injection failure of storage selector strategy.
### Description
Currently, all 4 implementations of `StorageLocationSelectorStrategy` requires a list of `StorageLocation` objects during construction. And `StorageLocation` differs from `StorageLocationConfig` deserialized from configuration file, and the former is instantiated by `SegmentLoaderLocalCacheManager`. This also means `StorageLocation` could not be injected into `StorageLocationSelectorStrategy` when they are being constructed.
In this PR,
1. the ctor of implementations of `StorageLocationSelectorStrategy` are removed, instead, a setter of storage location method is provided in this interface so that `SegmentLoaderLocalCacheManager` could pass the objects to the strategy object
2. based on the original code, configuration property should be `druid.segmentCache.locationSelectorStrategy.type`, related docs are also updated
3. some unit test cases are added to check whether these strategy objects are correctly instantiated from configuration properties.
<!-- If you are a committer, follow the PR action item checklist for committers:
https://github.com/apache/druid/blob/master/dev/committer-instructions.md#pr-and-issue-action-item-checklist-for-committers. -->
<hr>
This PR has:
- [X] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/licenses.yaml)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [X] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [X] been tested in a test Druid cluster.
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist above are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
<hr>
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-709959533
>
> @FrankChen021 I think the ITs should test that Druid uses the `StorageLocationSelector` that was configured by the user by looking at the user visible impact of changing this setting. So for example: A user who sets this to `mostAvailableSize` should see that segments are loaded on the historical with the most available size instead of the default which is least bytes used. Constructing a scenario where this behavior difference should occur may be tricky. I haven't had the time to think about how to construct this scenario yet. I will get back to you on my tests today. Thanks again for your patience
I see. It's an end-to-end test case. Usually it is tricky to set up an environment for such a case.
I also studied some IT cases, I found that test cases send HTTP requests to nodes to verify whether they run successfully or not. But for historical nodes, there're no such interfaces exposed for us to observe the behavior of selector strategy. As you say, it's a little tricky.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-707008126
Hi @suneet-s , do you have any further comments ?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10363:
URL: https://github.com/apache/druid/pull/10363#discussion_r485282869
##########
File path: server/src/main/java/org/apache/druid/segment/loading/LeastBytesUsedStorageLocationSelectorStrategy.java
##########
@@ -36,7 +38,13 @@
private List<StorageLocation> storageLocations;
- public LeastBytesUsedStorageLocationSelectorStrategy(List<StorageLocation> storageLocations)
+ @JsonCreator
+ public LeastBytesUsedStorageLocationSelectorStrategy()
+ {
+ }
+
+ @VisibleForTesting
+ LeastBytesUsedStorageLocationSelectorStrategy(List<StorageLocation> storageLocations)
Review comment:
Good idea.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s edited a comment on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
suneet-s edited a comment on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-705811222
> Hi @jihoonson @suneet-s , CI reports that compaction integration test fails while all other checks are ok. I don't know why, could you help me take a look at it?
@FrankChen021 It appears these tests are flaky. I re-triggered them and the job passed - could you file an issue for this flaky test please?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-693751147
The latest CI reports that
> Error: found 1 missing licenses. These licenses are reported, but missing in the registry
> druid_module: core, groupId: com.fasterxml.jackson.module, artifactId: jackson-module-guice, version: 2.10.2, license: Apache License version 2.0
` jackson-module-guice` is added to druid-server due to the failure of dependency check of previous CI check.
I don't know how to handle it. Do you have any idea ? @suneet-s @asdf2014
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 edited a comment on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 edited a comment on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-689487649
Hi @jihoonson @suneet-s , here's a clarification for the change of user facing configuration.
This PR has no code change with the configuration name, but a rectification of the doc from the wrong configuration item name of selector strategy named as `druid.segmentCache.locationSelectorStrategy` to `druid.segmentCache.locationSelectorStrategy.type`.
When I first checked the issue, the doc also made me confused and took me sometime on the question which was the right name.
Looking at the annotation of `StorageLocationSelectorStrategy`, it's bounded to a property named as `type` to determine which class should be used during deserialization.
```
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type", defaultImpl =
LeastBytesUsedStorageLocationSelectorStrategy.class)
@JsonSubTypes(value = {
```
Based upon the code, the right configuration name should `druid.segmentCache.locationSelectorStrategy.type`. The old implementation always fails to deserialize strategy object in default configuration mode but never throws any NPE exception because its getter method tries to initialize a default strategy object when it finds the strategy object is null.
I don't know why the `druid.segmentCache.locationSelectorStrategy` was put into the doc. Taking another class `BalancerStrategyFactory` for example, its doc corresponds to its property name in code.
```
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "strategy", defaultImpl = CostBalancerStrategyFactory.class)
@JsonSubTypes(value = {
@JsonSubTypes.Type(name = "diskNormalized", value = DiskNormalizedCostBalancerStrategyFactory.class),
@JsonSubTypes.Type(name = "cost", value = CostBalancerStrategyFactory.class),
@JsonSubTypes.Type(name = "cachingCost", value = CachingCostBalancerStrategyFactory.class),
@JsonSubTypes.Type(name = "random", value = RandomBalancerStrategyFactory.class),
})
public interface BalancerStrategyFactory
{
```
```
JsonConfigProvider.bind(binder, "druid.coordinator.balancer", BalancerStrategyFactory.class);
```
And the [doc](https://github.com/apache/druid/blob/master/docs/configuration/index.md#coordinator-operation) says the property name should be `druid.coordinator.balancer.strategy` instead of `druid.coordinator.balancer`
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10363:
URL: https://github.com/apache/druid/pull/10363#discussion_r484659870
##########
File path: server/src/test/java/org/apache/druid/segment/loading/StorageLocationSelectorStrategyTest.java
##########
@@ -256,4 +269,75 @@ public void testMostAvailableSizeLocationSelectorStrategy() throws Exception
Assert.assertEquals("The next element of the iterator should point to path local_storage_folder_1",
localStorageFolder1, loc3.getPath());
}
+
+ @Test
+ public void testDefaultSelectorStrategyConfig() throws Exception
Review comment:
`throws` declaration is useless here, maybe is introduced by code copy-paste. Been fixed.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10363:
URL: https://github.com/apache/druid/pull/10363#discussion_r500681350
##########
File path: server/src/main/java/org/apache/druid/guice/StorageNodeModule.java
##########
@@ -52,6 +55,7 @@ public void configure(Binder binder)
{
JsonConfigProvider.bind(binder, "druid.server", DruidServerConfig.class);
JsonConfigProvider.bind(binder, "druid.segmentCache", SegmentLoaderConfig.class);
+ JsonConfigProvider.bind(binder, "druid.segmentCache.locationSelector.strategy", StorageLocationSelectorStrategy.class);
Review comment:
Should this be `druid.segmentCache.locationSelector` instead?
##########
File path: server/src/test/java/org/apache/druid/segment/loading/StorageLocationSelectorStrategyTest.java
##########
@@ -256,4 +270,107 @@ public void testMostAvailableSizeLocationSelectorStrategy() throws Exception
Assert.assertEquals("The next element of the iterator should point to path local_storage_folder_1",
localStorageFolder1, loc3.getPath());
}
+
+ @Test
+ public void testDefaultSelectorStrategyConfig()
+ {
+ //no druid.segmentCache.locationSelector.strategy specified, the default will be used
+ final Properties props = new Properties();
+ props.setProperty("druid.segmentCache.locations", "[{\"path\": \"/tmp/druid/indexCache\"}]");
+
+ StorageLocationSelectorStrategy strategy = makeInjectorWithProperties(props).getInstance(StorageLocationSelectorStrategy.class);
+ Assert.assertEquals(LeastBytesUsedStorageLocationSelectorStrategy.class,
+ strategy.getClass());
+ Assert.assertEquals("/tmp/druid/indexCache", strategy.getLocations().next().getPath().getAbsolutePath());
+ }
+
+ @Test
+ public void testRoundRobinSelectorStrategyConfig()
+ {
+ final Properties props = new Properties();
+ props.setProperty("druid.segmentCache.locations", "[{\"path\": \"/tmp/druid/indexCache\"}]");
+ props.setProperty("druid.segmentCache.locationSelector.strategy", "roundRobin");
+
+ Injector injector = makeInjectorWithProperties(props);
+ StorageLocationSelectorStrategy strategy = injector.getInstance(StorageLocationSelectorStrategy.class);
+
+ Assert.assertEquals(RoundRobinStorageLocationSelectorStrategy.class,
+ strategy.getClass());
+ Assert.assertEquals("/tmp/druid/indexCache", strategy.getLocations().next().getPath().getAbsolutePath());
+ }
+
+ @Test
+ public void testLeastBytesUsedSelectorStrategyConfig()
+ {
+ final Properties props = new Properties();
+ props.setProperty("druid.segmentCache.locations", "[{\"path\": \"/tmp/druid/indexCache\"}]");
+ props.setProperty("druid.segmentCache.locationSelector.strategy", "leastBytesUsed");
+
+ Injector injector = makeInjectorWithProperties(props);
+ StorageLocationSelectorStrategy strategy = injector.getInstance(StorageLocationSelectorStrategy.class);
+
+ Assert.assertEquals(LeastBytesUsedStorageLocationSelectorStrategy.class,
+ strategy.getClass());
+ Assert.assertEquals("/tmp/druid/indexCache", strategy.getLocations().next().getPath().getAbsolutePath());
+ }
+
+ @Test
+ public void testRandomSelectorStrategyConfig()
+ {
+ final Properties props = new Properties();
+ props.setProperty("druid.segmentCache.locations", "[{\"path\": \"/tmp/druid/indexCache\"}]");
+ props.setProperty("druid.segmentCache.locationSelector.strategy", "random");
+
+ Injector injector = makeInjectorWithProperties(props);
+ StorageLocationSelectorStrategy strategy = injector.getInstance(StorageLocationSelectorStrategy.class);
+
+ Assert.assertEquals(RandomStorageLocationSelectorStrategy.class,
+ strategy.getClass());
+ Assert.assertEquals("/tmp/druid/indexCache", strategy.getLocations().next().getPath().getAbsolutePath());
+ }
+
+ @Test
+ public void testMostAvailableSizeSelectorStrategyConfig()
+ {
+ final Properties props = new Properties();
+ props.setProperty("druid.segmentCache.locationSelector.strategy", "mostAvailableSize");
+ props.setProperty("druid.segmentCache.locations", "[{\"path\": \"/tmp/druid/indexCache\"}]");
+
+ Injector injector = makeInjectorWithProperties(props);
+ StorageLocationSelectorStrategy strategy = injector.getInstance(StorageLocationSelectorStrategy.class);
+
+ Assert.assertEquals(MostAvailableSizeStorageLocationSelectorStrategy.class,
+ strategy.getClass());
+ Assert.assertEquals("/tmp/druid/indexCache", strategy.getLocations().next().getPath().getAbsolutePath());
+ }
+
+ private Injector makeInjectorWithProperties(final Properties props)
+ {
+ return Guice.createInjector(
+ new Module()
+ {
+ @Override
+ public void configure(Binder binder)
+ {
+ //ObjectMapperModule introduce Guice injector for jackson
+ binder.install(new ObjectMapperModule()
+ .withObjectMapper(new DefaultObjectMapper()));
+ binder.install(new DruidGuiceExtensions());
+
+ binder.bind(Validator.class).toInstance(Validation.buildDefaultValidatorFactory().getValidator());
+ binder.bind(JsonConfigurator.class).in(LazySingleton.class);
+ binder.bind(Properties.class).toInstance(props);
+
+ JsonConfigProvider.bind(binder, "druid.segmentCache", SegmentLoaderConfig.class);
+ JsonConfigProvider.bind(binder, "druid.segmentCache.locationSelector", StorageLocationSelectorStrategy.class);
Review comment:
This binding is different from what actual module binds. I think this is why we missed the wrong binding. Can we use the same `StorageNodeModule` here? Or can we add a helper method which does the proper binding for both `StorageNodeModule` and tests?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10363:
URL: https://github.com/apache/druid/pull/10363#discussion_r490141864
##########
File path: server/src/main/java/org/apache/druid/segment/loading/SegmentLoaderConfig.java
##########
@@ -54,9 +54,6 @@
@JsonProperty("numBootstrapThreads")
private Integer numBootstrapThreads = null;
- @JsonProperty("locationSelectorStrategy")
- private StorageLocationSelectorStrategy locationSelectorStrategy;
Review comment:
This is because `StorageLocationStrategy` depends on `SegmentLoaderConfig.locations`. If `StorageLocationStrategy` is placed here, when `SegmentLoaderConfig` is being deserialized, `locations` required by `StorageLocationStrategy` can't be found and injected into it.
After moving this property out of `SegmentLoaderConfig`, both `SegmentLoaderConfig` and `StorageLocationStrategy` are deserialized during construction of `SegmentLoaderLocalCacheManager`, and jackson could find the `locations` objects required by strategy object through google guice framework to create strategy object correctly.
Using the configuration name without `type` I think is wrong. Please take a look at this configuration `druid.coordinator.balancer.strategy`, or the clarification on the change of configuration name I left above.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-704432219
@suneet-s do you have more comments?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s edited a comment on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
suneet-s edited a comment on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-705811222
> Hi @jihoonson @suneet-s , CI reports that compaction integration test fails while all other checks are ok. I don't know why, could you help me take a look at it?
@FrankChen021 It appears these tests are flaky. I re-triggered them and the job passed - could you file an issue for this flaky test please?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-740802506
Thanks @FrankChen021 - looks like someone else beat me to the re-trigger.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-691226275
@FrankChen021 I'll look through these changes over the next couple of days and get back to you. Thanks for the fix and your patience :)
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10363:
URL: https://github.com/apache/druid/pull/10363#discussion_r485284601
##########
File path: docs/configuration/index.md
##########
@@ -1379,7 +1379,7 @@ These Historical configurations can be defined in the `historical/runtime.proper
|Property|Description|Default|
|--------|-----------|-------|
|`druid.segmentCache.locations`|Segments assigned to a Historical process are first stored on the local file system (in a disk cache) and then served by the Historical process. These locations define where that local cache resides. This value cannot be NULL or EMPTY. Here is an example `druid.segmentCache.locations=[{"path": "/mnt/druidSegments", "maxSize": "10k", "freeSpacePercent": 1.0}]`. "freeSpacePercent" is optional, if provided then enforces that much of free disk partition space while storing segments. But, it depends on File.getTotalSpace() and File.getFreeSpace() methods, so enable if only if they work for your File System.| none |
-|`druid.segmentCache.locationSelectorStrategy`|The strategy used to select a location from the configured `druid.segmentCache.locations` for segment distribution. Possible values are `leastBytesUsed`, `roundRobin`, `random`, or `mostAvailableSize`. |leastBytesUsed|
+|`druid.segmentCache.locationSelectorStrategy.type`|The strategy used to select a location from the configured `druid.segmentCache.locations` for segment distribution. Possible values are `leastBytesUsed`, `roundRobin`, `random`, or `mostAvailableSize`. |leastBytesUsed|
Review comment:
It's a fix. According to original code, annotation on `StorageLocationSelectorStrategy` indicates the name of json property is `type`. When using `druid.segmentCache.locationSelectorStrategy`, jackson tries to find ctor with a String parameter , which causes the issue.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10363:
URL: https://github.com/apache/druid/pull/10363#discussion_r493131538
##########
File path: server/src/main/java/org/apache/druid/segment/loading/SegmentLoaderConfig.java
##########
@@ -54,9 +54,6 @@
@JsonProperty("numBootstrapThreads")
private Integer numBootstrapThreads = null;
- @JsonProperty("locationSelectorStrategy")
- private StorageLocationSelectorStrategy locationSelectorStrategy;
Review comment:
Sorry for the late reply.
Your suggestion makes the naming more meaningful. I'll update this PR later this day.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10363:
URL: https://github.com/apache/druid/pull/10363#discussion_r485188552
##########
File path: server/src/main/java/org/apache/druid/segment/loading/LeastBytesUsedStorageLocationSelectorStrategy.java
##########
@@ -36,7 +38,13 @@
private List<StorageLocation> storageLocations;
- public LeastBytesUsedStorageLocationSelectorStrategy(List<StorageLocation> storageLocations)
+ @JsonCreator
+ public LeastBytesUsedStorageLocationSelectorStrategy()
+ {
+ }
+
+ @VisibleForTesting
+ LeastBytesUsedStorageLocationSelectorStrategy(List<StorageLocation> storageLocations)
Review comment:
I think this approach is brittle and might break when a new implementation of `StorageLocationSelectorStrategy` is added. Instead, I think we should make `List<StorageLocation> storageLocations` injectable instead via a module, something like
```
@Provides
@Singleton
public List<StorageLocation> provideStorageLocations(SegmentLoaderConfig config)
{
this.locations = new ArrayList<>();
for (StorageLocationConfig locationConfig : config.getLocations()) {
locations.add(
new StorageLocation(
locationConfig.getPath(),
locationConfig.getMaxSize(),
locationConfig.getFreeSpacePercent()
)
);
return locations;
}
```
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-707233107
@suneet-s Got it. I'm not familiar with IT. One thing I don't understand is what we should expect from the integration tests. To verify if the right type of selector object injected for corresponding configuration?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10363:
URL: https://github.com/apache/druid/pull/10363#discussion_r484658436
##########
File path: server/src/main/java/org/apache/druid/segment/loading/LeastBytesUsedStorageLocationSelectorStrategy.java
##########
@@ -32,11 +34,17 @@
public class LeastBytesUsedStorageLocationSelectorStrategy implements StorageLocationSelectorStrategy
{
private static final Ordering<StorageLocation> ORDERING = Ordering.from(Comparator
- .comparingLong(StorageLocation::currSizeBytes));
+ .comparingLong(StorageLocation::currSizeBytes));
Review comment:
It's accidentally formatted by IDEA. Been fixed.
##########
File path: server/src/main/java/org/apache/druid/segment/loading/MostAvailableSizeStorageLocationSelectorStrategy.java
##########
@@ -32,12 +34,18 @@
public class MostAvailableSizeStorageLocationSelectorStrategy implements StorageLocationSelectorStrategy
{
private static final Ordering<StorageLocation> ORDERING = Ordering.from(Comparator
- .comparingLong(StorageLocation::availableSizeBytes)
- .reversed());
+ .comparingLong(StorageLocation::availableSizeBytes)
Review comment:
It's accidentally formatted by IDEA. Been fixed.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10363:
URL: https://github.com/apache/druid/pull/10363#discussion_r484660122
##########
File path: server/src/test/java/org/apache/druid/segment/loading/StorageLocationSelectorStrategyTest.java
##########
@@ -256,4 +269,75 @@ public void testMostAvailableSizeLocationSelectorStrategy() throws Exception
Assert.assertEquals("The next element of the iterator should point to path local_storage_folder_1",
localStorageFolder1, loc3.getPath());
}
+
+ @Test
+ public void testDefaultSelectorStrategyConfig() throws Exception
+ {
+ //no druid.segmentCache.locationSelectorStrategy.type specified
+ final Properties props = new Properties();
+ SegmentLoaderConfig loaderConfig = makeInjectorWithProperties(props).getInstance(SegmentLoaderConfig.class);
+ Assert.assertEquals(LeastBytesUsedStorageLocationSelectorStrategy.class,
+ loaderConfig.getStorageLocationSelectorStrategy().getClass());
+ }
+
+ @Test
+ public void testRoundRobinSelectorStrategyConfig() throws Exception
+ {
+ final Properties props = new Properties();
+ props.put("druid.segmentCache.locationSelectorStrategy.type", "roundRobin");
Review comment:
Been fixed.
##########
File path: server/src/main/java/org/apache/druid/segment/loading/SegmentLoaderLocalCacheManager.java
##########
@@ -99,7 +99,10 @@ public SegmentLoaderLocalCacheManager(
)
);
}
- this.strategy = config.getStorageLocationSelectorStrategy(locations);
+
+ this.strategy = config.getStorageLocationSelectorStrategy();
+ this.strategy.setLocations(locations);
+ log.info("using storage location strategy: %s", this.strategy.getClass().getSimpleName());
Review comment:
Been fixed.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10363:
URL: https://github.com/apache/druid/pull/10363#discussion_r493131538
##########
File path: server/src/main/java/org/apache/druid/segment/loading/SegmentLoaderConfig.java
##########
@@ -54,9 +54,6 @@
@JsonProperty("numBootstrapThreads")
private Integer numBootstrapThreads = null;
- @JsonProperty("locationSelectorStrategy")
- private StorageLocationSelectorStrategy locationSelectorStrategy;
Review comment:
Sorry for the late reply.
Your suggestion makes the naming more meaningful. I'll update this PR later this day.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-690837225
@jihoonson @suneet-s I come up with an idea that requires no change to the existing configuration.
1. bind configuration path `druid.segmentCache` to `StorageLocationSelectorStrategy` directly
2. change the value of property in JsonTypeInfo annotation of `StorageLocationSelectorStrategy` from `type` to `locationSelectorStrategy`
The disadvantage is that all properties extended by implementations of `StorageLocationSelectorStrategy` in the future will all be put under `druid.segmentCache`, which means these properties are mixed up with properties of `SegmentLoaderConfig`. This approach might cause some confusion.
What do u think ?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-731479320
@FrankChen021 - I got pulled in to some other issues and haven't had the time to look at this yet. I will look at this over the weekend and get back to you.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10363:
URL: https://github.com/apache/druid/pull/10363#discussion_r484659412
##########
File path: server/src/main/java/org/apache/druid/segment/loading/RoundRobinStorageLocationSelectorStrategy.java
##########
@@ -32,19 +35,25 @@
*/
public class RoundRobinStorageLocationSelectorStrategy implements StorageLocationSelectorStrategy
{
-
- private final List<StorageLocation> storageLocations;
+ private List<StorageLocation> storageLocations = null;
Review comment:
Been fixed.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-740380772
Hi @suneet-s The conflict has been resolved by rebasing the branch onto the latest master. But CI failed, I checked it, and it seems that it's not related to the changes in PR. Could you re-trigger it to see if it passes ?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10363:
URL: https://github.com/apache/druid/pull/10363#discussion_r485190171
##########
File path: docs/configuration/index.md
##########
@@ -1379,7 +1379,7 @@ These Historical configurations can be defined in the `historical/runtime.proper
|Property|Description|Default|
|--------|-----------|-------|
|`druid.segmentCache.locations`|Segments assigned to a Historical process are first stored on the local file system (in a disk cache) and then served by the Historical process. These locations define where that local cache resides. This value cannot be NULL or EMPTY. Here is an example `druid.segmentCache.locations=[{"path": "/mnt/druidSegments", "maxSize": "10k", "freeSpacePercent": 1.0}]`. "freeSpacePercent" is optional, if provided then enforces that much of free disk partition space while storing segments. But, it depends on File.getTotalSpace() and File.getFreeSpace() methods, so enable if only if they work for your File System.| none |
-|`druid.segmentCache.locationSelectorStrategy`|The strategy used to select a location from the configured `druid.segmentCache.locations` for segment distribution. Possible values are `leastBytesUsed`, `roundRobin`, `random`, or `mostAvailableSize`. |leastBytesUsed|
+|`druid.segmentCache.locationSelectorStrategy.type`|The strategy used to select a location from the configured `druid.segmentCache.locations` for segment distribution. Possible values are `leastBytesUsed`, `roundRobin`, `random`, or `mostAvailableSize`. |leastBytesUsed|
Review comment:
Is this a doc fix? I don't see an associated change in the code. Am I missing something?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10363:
URL: https://github.com/apache/druid/pull/10363#discussion_r490442882
##########
File path: server/src/main/java/org/apache/druid/segment/loading/SegmentLoaderConfig.java
##########
@@ -54,9 +54,6 @@
@JsonProperty("numBootstrapThreads")
private Integer numBootstrapThreads = null;
- @JsonProperty("locationSelectorStrategy")
- private StorageLocationSelectorStrategy locationSelectorStrategy;
Review comment:
Got it. Then, how about using a similar naming to the coordinator balancer? For example, we can bind `StorageLocationSelectorStrategy` to `druid.segmentCache.locationSelector`, and use a `strategy` property name for `StorageLocationSelectorStrategy` instead of `type`.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10363:
URL: https://github.com/apache/druid/pull/10363#discussion_r500902483
##########
File path: server/src/test/java/org/apache/druid/segment/loading/StorageLocationSelectorStrategyTest.java
##########
@@ -256,4 +270,107 @@ public void testMostAvailableSizeLocationSelectorStrategy() throws Exception
Assert.assertEquals("The next element of the iterator should point to path local_storage_folder_1",
localStorageFolder1, loc3.getPath());
}
+
+ @Test
+ public void testDefaultSelectorStrategyConfig()
+ {
+ //no druid.segmentCache.locationSelector.strategy specified, the default will be used
+ final Properties props = new Properties();
+ props.setProperty("druid.segmentCache.locations", "[{\"path\": \"/tmp/druid/indexCache\"}]");
+
+ StorageLocationSelectorStrategy strategy = makeInjectorWithProperties(props).getInstance(StorageLocationSelectorStrategy.class);
+ Assert.assertEquals(LeastBytesUsedStorageLocationSelectorStrategy.class,
+ strategy.getClass());
+ Assert.assertEquals("/tmp/druid/indexCache", strategy.getLocations().next().getPath().getAbsolutePath());
+ }
+
+ @Test
+ public void testRoundRobinSelectorStrategyConfig()
+ {
+ final Properties props = new Properties();
+ props.setProperty("druid.segmentCache.locations", "[{\"path\": \"/tmp/druid/indexCache\"}]");
+ props.setProperty("druid.segmentCache.locationSelector.strategy", "roundRobin");
+
+ Injector injector = makeInjectorWithProperties(props);
+ StorageLocationSelectorStrategy strategy = injector.getInstance(StorageLocationSelectorStrategy.class);
+
+ Assert.assertEquals(RoundRobinStorageLocationSelectorStrategy.class,
+ strategy.getClass());
+ Assert.assertEquals("/tmp/druid/indexCache", strategy.getLocations().next().getPath().getAbsolutePath());
+ }
+
+ @Test
+ public void testLeastBytesUsedSelectorStrategyConfig()
+ {
+ final Properties props = new Properties();
+ props.setProperty("druid.segmentCache.locations", "[{\"path\": \"/tmp/druid/indexCache\"}]");
+ props.setProperty("druid.segmentCache.locationSelector.strategy", "leastBytesUsed");
+
+ Injector injector = makeInjectorWithProperties(props);
+ StorageLocationSelectorStrategy strategy = injector.getInstance(StorageLocationSelectorStrategy.class);
+
+ Assert.assertEquals(LeastBytesUsedStorageLocationSelectorStrategy.class,
+ strategy.getClass());
+ Assert.assertEquals("/tmp/druid/indexCache", strategy.getLocations().next().getPath().getAbsolutePath());
+ }
+
+ @Test
+ public void testRandomSelectorStrategyConfig()
+ {
+ final Properties props = new Properties();
+ props.setProperty("druid.segmentCache.locations", "[{\"path\": \"/tmp/druid/indexCache\"}]");
+ props.setProperty("druid.segmentCache.locationSelector.strategy", "random");
+
+ Injector injector = makeInjectorWithProperties(props);
+ StorageLocationSelectorStrategy strategy = injector.getInstance(StorageLocationSelectorStrategy.class);
+
+ Assert.assertEquals(RandomStorageLocationSelectorStrategy.class,
+ strategy.getClass());
+ Assert.assertEquals("/tmp/druid/indexCache", strategy.getLocations().next().getPath().getAbsolutePath());
+ }
+
+ @Test
+ public void testMostAvailableSizeSelectorStrategyConfig()
+ {
+ final Properties props = new Properties();
+ props.setProperty("druid.segmentCache.locationSelector.strategy", "mostAvailableSize");
+ props.setProperty("druid.segmentCache.locations", "[{\"path\": \"/tmp/druid/indexCache\"}]");
+
+ Injector injector = makeInjectorWithProperties(props);
+ StorageLocationSelectorStrategy strategy = injector.getInstance(StorageLocationSelectorStrategy.class);
+
+ Assert.assertEquals(MostAvailableSizeStorageLocationSelectorStrategy.class,
+ strategy.getClass());
+ Assert.assertEquals("/tmp/druid/indexCache", strategy.getLocations().next().getPath().getAbsolutePath());
+ }
+
+ private Injector makeInjectorWithProperties(final Properties props)
+ {
+ return Guice.createInjector(
+ new Module()
+ {
+ @Override
+ public void configure(Binder binder)
+ {
+ //ObjectMapperModule introduce Guice injector for jackson
+ binder.install(new ObjectMapperModule()
+ .withObjectMapper(new DefaultObjectMapper()));
+ binder.install(new DruidGuiceExtensions());
+
+ binder.bind(Validator.class).toInstance(Validation.buildDefaultValidatorFactory().getValidator());
+ binder.bind(JsonConfigurator.class).in(LazySingleton.class);
+ binder.bind(Properties.class).toInstance(props);
+
+ JsonConfigProvider.bind(binder, "druid.segmentCache", SegmentLoaderConfig.class);
+ JsonConfigProvider.bind(binder, "druid.segmentCache.locationSelector", StorageLocationSelectorStrategy.class);
Review comment:
The reason why `StorageModule` is not used here is because there're some extra dependencies in `StorageModule`, which might introduce lots of injection code in test cases. A helper method is extracted to do the correct binding.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-690837225
@jihoonson @suneet-s I come up with an idea that requires no change to the existing configuration.
1. bind configuration path `druid.segmentCache` to `StorageLocationSelectorStrategy` directly
2. change the value of property in JsonTypeInfo annotation of `StorageLocationSelectorStrategy` from `type` to `locationSelectorStrategy`
The disadvantage is that all properties extended by implementations of `StorageLocationSelectorStrategy` in the future will all be put under `druid.segmentCache`, which means these properties are mixed up with properties of `SegmentLoaderConfig`. This approach might cause some confusion.
What do u think ?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10363:
URL: https://github.com/apache/druid/pull/10363#discussion_r484659353
##########
File path: server/src/main/java/org/apache/druid/segment/loading/RandomStorageLocationSelectorStrategy.java
##########
@@ -31,9 +34,15 @@
public class RandomStorageLocationSelectorStrategy implements StorageLocationSelectorStrategy
{
- private final List<StorageLocation> storageLocations;
+ private List<StorageLocation> storageLocations = null;
Review comment:
IDEA reports an assignment of null is required due to some customized check rules. Been fixed.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-705003941
@FrankChen021 Can you describe how you tested this in your test cluster. What was the user-visible behavior before and after this change so that we can update the release notes if needed.
I didn't see any integration tests added for this change, so it's possible that someone might break this behavior again in the future - would it be possible to add integration tests?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-705021331
Hi @suneet-s , in this PR a log is added in the ctor of `SegmentLoaderLocalCacheManager` to print the class name of the strategy object. The log shows as follows if `druid.segmentCache.locationSelector.strategy` is set to `roundRobin`
> 2020-10-07T15:22:55,251` INFO [main] org.apache.druid.segment.loading.SegmentLoaderLocalCacheManager - Using storage location strategy: [RoundRobinStorageLocationSelectorStrategy]
In this way, I know whether the configuration takes effect.
As the test cases, I've added some unit test cases to test whether this configuration takes effect by setting `druid.segmentCache.locationSelector.strategy` to different values. I think these cases would guard our code in case of future modification.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10363:
URL: https://github.com/apache/druid/pull/10363#discussion_r485428861
##########
File path: server/src/main/java/org/apache/druid/segment/loading/LeastBytesUsedStorageLocationSelectorStrategy.java
##########
@@ -36,7 +38,13 @@
private List<StorageLocation> storageLocations;
- public LeastBytesUsedStorageLocationSelectorStrategy(List<StorageLocation> storageLocations)
+ @JsonCreator
+ public LeastBytesUsedStorageLocationSelectorStrategy()
+ {
+ }
+
+ @VisibleForTesting
+ LeastBytesUsedStorageLocationSelectorStrategy(List<StorageLocation> storageLocations)
Review comment:
Hi @suneet-s I check the code and find that this approach is a little bit complex for the existing code.
Storage selector strategy object is constructed during deserialization of `SegmentLoaderConfig`, and only after the construction of `SegmentLoaderConfig`, could it be possible to inject `SegmentLoaderConfig` to other objects to get list of `StorageLocationConfig` inside the object.
If we want to inject `StorageLocation` objects as the way you suggest, the strategy object must be separated from `SegmentLoaderConfig` into another new config class so that both `SegmentLoaderConfig` and this new config class can be injected into `SegmentLocalCacheManager`. There will be lots of test cases involved to change to meet the new ctor of `SegmentLocalCacheManager`. So I think these change involve more complexity.
Back to the concern you mentioned, I think there's no need to worry that new implementation would break the constraints. Because if it breaks, the problem would be easily detected during unit test or integrated test.
What do you think ?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s merged pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
suneet-s merged pull request #10363:
URL: https://github.com/apache/druid/pull/10363
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-693914470
> The latest CI reports that
>
> > Error: found 1 missing licenses. These licenses are reported, but missing in the registry
> > druid_module: core, groupId: com.fasterxml.jackson.module, artifactId: jackson-module-guice, version: 2.10.2, license: Apache License version 2.0
>
> ` jackson-module-guice` is added to druid-server due to the failure of dependency check of previous CI check.
>
> I don't know how to handle it. Do you have any idea ? @suneet-s @asdf2014
This is because a mismatch between the version used in `pom.xml` and the one registered in `licenses.yaml`. I'm not sure why we haven't seen this error before.. But you can fix it by removing [this entry](https://github.com/apache/druid/blob/master/licenses.yaml#L4105-L4111) and adding `jackson-module-guice` to [here](https://github.com/apache/druid/blob/master/licenses.yaml#L233).
Other CI failures look unrelated. I just restarted them.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-705811222
> Hi @jihoonson @suneet-s , CI reports that compaction integration test fails while all other checks are ok. I don't know why, could you help me take a look at it?
It appears these tests are flaky. I re-triggered them and the job passed
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-691226275
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-689487649
Hi @jihoonson @suneet-s , here's a clarification for the change of user facing configuration.
This PR has no code change with the configuration name, but a rectification of the doc from the wrong configuration item name of selector strategy named as `druid.segmentCache.locationSelectorStrategy` to `druid.segmentCache.locationSelectorStrategy.type`.
When I first checked the issue, the doc also made me confused and took me sometime on the question which was the right name.
Looking at the annotation of `StorageLocationSelectorStrategy`, it's bounded to a property named as `type` to determine which class should be used during deserialization.
```
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type", defaultImpl =
LeastBytesUsedStorageLocationSelectorStrategy.class)
@JsonSubTypes(value = {
```
Based upon the code, the right configuration name should `druid.segmentCache.locationSelectorStrategy.type`. The old implementation always fails to deserialize strategy object in default configuration mode but never throws any NPE exception because its getter method tries to initialize a default strategy object when it finds the strategy object is null.
I don't know why the `druid.segmentCache.locationSelectorStrategy` was put into the doc. Taking another class `BalancerStrategyFactory` for example, its doc corresponding to its property name in code.
```
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "strategy", defaultImpl = CostBalancerStrategyFactory.class)
@JsonSubTypes(value = {
@JsonSubTypes.Type(name = "diskNormalized", value = DiskNormalizedCostBalancerStrategyFactory.class),
@JsonSubTypes.Type(name = "cost", value = CostBalancerStrategyFactory.class),
@JsonSubTypes.Type(name = "cachingCost", value = CachingCostBalancerStrategyFactory.class),
@JsonSubTypes.Type(name = "random", value = RandomBalancerStrategyFactory.class),
})
public interface BalancerStrategyFactory
{
```
```
JsonConfigProvider.bind(binder, "druid.coordinator.balancer", BalancerStrategyFactory.class);
```
And the [doc](https://github.com/apache/druid/blob/master/docs/configuration/index.md#coordinator-operation) says the property name should be `druid.coordinator.balancer.strategy` instead of `druid.coordinator.balancer`
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-691226275
@FrankChen021 I'll look through these changes over the next couple of days and get back to you. Thanks for the fix and your patience :)
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson edited a comment on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
jihoonson edited a comment on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-693914470
> The latest CI reports that
>
> > Error: found 1 missing licenses. These licenses are reported, but missing in the registry
> > druid_module: core, groupId: com.fasterxml.jackson.module, artifactId: jackson-module-guice, version: 2.10.2, license: Apache License version 2.0
>
> ` jackson-module-guice` is added to druid-server due to the failure of dependency check of previous CI check.
>
> I don't know how to handle it. Do you have any idea ? @suneet-s @asdf2014
This is because of a mismatch between the version used in `pom.xml` and the one registered in `licenses.yaml`. I'm not sure why we haven't seen this error before.. But you can fix it by removing [this entry](https://github.com/apache/druid/blob/master/licenses.yaml#L4105-L4111) and adding `jackson-module-guice` to [here](https://github.com/apache/druid/blob/master/licenses.yaml#L233).
Other CI failures look unrelated. I just restarted them.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-707205182
> Hi @suneet-s , do you have any further comments ?
@FrankChen021 Sorry for the delay in this review. Since there are no integration tests, I'm trying to see what would happen on an upgrade, and whether or not this change would introduce a change in behavior. If you have integration tests that prove nothing has changed on the upgrade path, it would make this review a lot faster. Thanks for your patience
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-690837225
@jihoonson @suneet-s I come up with an idea that requires no change to the existing configuration.
1. bind configuration path `druid.segmentCache` to `StorageLocationSelectorStrategy` directly
2. change the value of property in JsonTypeInfo annotation of `StorageLocationSelectorStrategy` from `type` to `locationSelectorStrategy`
The disadvantage is that all properties extended by implementations of `StorageLocationSelectorStrategy` in the future will all be put under `druid.segmentCache`, which means these properties are mixed up with properties of `SegmentLoaderConfig`. This approach might cause some confusion.
What do u think ?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-691226275
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-731087817
Hi @suneet-s , Is this PR ready to be merged ? or is there anything I need to do ?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10363:
URL: https://github.com/apache/druid/pull/10363#discussion_r489986586
##########
File path: server/src/main/java/org/apache/druid/segment/loading/SegmentLoaderConfig.java
##########
@@ -54,9 +54,6 @@
@JsonProperty("numBootstrapThreads")
private Integer numBootstrapThreads = null;
- @JsonProperty("locationSelectorStrategy")
- private StorageLocationSelectorStrategy locationSelectorStrategy;
Review comment:
I think the previous implementation is better since you can use the configuration name `druid.segmentCache.locationSelectorStrategy` without `type`. Is there a reason that `locationSelectorStrategy` cannot be here?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-689127725
Tagged "Design Review" since it changes user-facing configurations.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-708525495
> @suneet-s Got it. I'm not familiar with IT. One thing I don't understand is what we should expect from the integration tests. To verify if the right type of selector object injected for corresponding configuration?
@FrankChen021 I think the ITs should test that Druid uses the `StorageLocationSelector` that was configured by the user by looking at the user visible impact of changing this setting. So for example: A user who sets this to `mostAvailableSize` should see that segments are loaded on the historical with the most available size instead of the default which is least bytes used. Constructing a scenario where this behavior difference should occur may be tricky. I haven't had the time to think about how to construct this scenario yet. I will get back to you on my tests today. Thanks again for your patience
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-705811222
> Hi @jihoonson @suneet-s , CI reports that compaction integration test fails while all other checks are ok. I don't know why, could you help me take a look at it?
It appears these tests are flaky. I re-triggered them and the job passed
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] asdf2014 commented on a change in pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
asdf2014 commented on a change in pull request #10363:
URL: https://github.com/apache/druid/pull/10363#discussion_r484637958
##########
File path: server/src/main/java/org/apache/druid/segment/loading/LeastBytesUsedStorageLocationSelectorStrategy.java
##########
@@ -32,11 +34,17 @@
public class LeastBytesUsedStorageLocationSelectorStrategy implements StorageLocationSelectorStrategy
{
private static final Ordering<StorageLocation> ORDERING = Ordering.from(Comparator
- .comparingLong(StorageLocation::currSizeBytes));
+ .comparingLong(StorageLocation::currSizeBytes));
Review comment:
Please roll back useless code formatting.
##########
File path: server/src/main/java/org/apache/druid/segment/loading/RoundRobinStorageLocationSelectorStrategy.java
##########
@@ -32,19 +35,25 @@
*/
public class RoundRobinStorageLocationSelectorStrategy implements StorageLocationSelectorStrategy
{
-
- private final List<StorageLocation> storageLocations;
+ private List<StorageLocation> storageLocations = null;
Review comment:
The default values of reference types in Java are all null, so assigning null here should be meaningless.
##########
File path: server/src/main/java/org/apache/druid/segment/loading/MostAvailableSizeStorageLocationSelectorStrategy.java
##########
@@ -32,12 +34,18 @@
public class MostAvailableSizeStorageLocationSelectorStrategy implements StorageLocationSelectorStrategy
{
private static final Ordering<StorageLocation> ORDERING = Ordering.from(Comparator
- .comparingLong(StorageLocation::availableSizeBytes)
- .reversed());
+ .comparingLong(StorageLocation::availableSizeBytes)
Review comment:
Please roll back useless code formatting.
##########
File path: server/src/main/java/org/apache/druid/segment/loading/RandomStorageLocationSelectorStrategy.java
##########
@@ -31,9 +34,15 @@
public class RandomStorageLocationSelectorStrategy implements StorageLocationSelectorStrategy
{
- private final List<StorageLocation> storageLocations;
+ private List<StorageLocation> storageLocations = null;
Review comment:
The default values of reference types in Java are all null, so assigning null here should be meaningless.
##########
File path: server/src/main/java/org/apache/druid/segment/loading/SegmentLoaderLocalCacheManager.java
##########
@@ -99,7 +99,10 @@ public SegmentLoaderLocalCacheManager(
)
);
}
- this.strategy = config.getStorageLocationSelectorStrategy(locations);
+
+ this.strategy = config.getStorageLocationSelectorStrategy();
+ this.strategy.setLocations(locations);
+ log.info("using storage location strategy: %s", this.strategy.getClass().getSimpleName());
Review comment:
Generally, the first letter of the exception information needs to be capitalized. In addition, %s needs to be surrounded by square brackets. So please use `Using storage location strategy: [%s]`.
##########
File path: server/src/test/java/org/apache/druid/segment/loading/StorageLocationSelectorStrategyTest.java
##########
@@ -256,4 +269,75 @@ public void testMostAvailableSizeLocationSelectorStrategy() throws Exception
Assert.assertEquals("The next element of the iterator should point to path local_storage_folder_1",
localStorageFolder1, loc3.getPath());
}
+
+ @Test
+ public void testDefaultSelectorStrategyConfig() throws Exception
Review comment:
In the newly added test cases, the `throw Exception` statement needs to be removed to pass the travis CI.
##########
File path: server/src/test/java/org/apache/druid/segment/loading/StorageLocationSelectorStrategyTest.java
##########
@@ -256,4 +269,75 @@ public void testMostAvailableSizeLocationSelectorStrategy() throws Exception
Assert.assertEquals("The next element of the iterator should point to path local_storage_folder_1",
localStorageFolder1, loc3.getPath());
}
+
+ @Test
+ public void testDefaultSelectorStrategyConfig() throws Exception
+ {
+ //no druid.segmentCache.locationSelectorStrategy.type specified
+ final Properties props = new Properties();
+ SegmentLoaderConfig loaderConfig = makeInjectorWithProperties(props).getInstance(SegmentLoaderConfig.class);
+ Assert.assertEquals(LeastBytesUsedStorageLocationSelectorStrategy.class,
+ loaderConfig.getStorageLocationSelectorStrategy().getClass());
+ }
+
+ @Test
+ public void testRoundRobinSelectorStrategyConfig() throws Exception
+ {
+ final Properties props = new Properties();
+ props.put("druid.segmentCache.locationSelectorStrategy.type", "roundRobin");
Review comment:
Although `Properties` inherits from `Hashtable` and can call the put method, it is recommended to use the setProperty method. In this way, you can forcibly restrict the incoming Key and Value of the String type to avoid an error when you call store or save again after passing in non-String data.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10363:
URL: https://github.com/apache/druid/pull/10363#issuecomment-705025377
Hi @jihoonson @suneet-s , CI reports that compaction integration test fails while all other checks are ok. I don't know why, could you help me take a look at it?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10363:
URL: https://github.com/apache/druid/pull/10363#discussion_r500904061
##########
File path: server/src/main/java/org/apache/druid/guice/StorageNodeModule.java
##########
@@ -52,6 +55,7 @@ public void configure(Binder binder)
{
JsonConfigProvider.bind(binder, "druid.server", DruidServerConfig.class);
JsonConfigProvider.bind(binder, "druid.segmentCache", SegmentLoaderConfig.class);
+ JsonConfigProvider.bind(binder, "druid.segmentCache.locationSelector.strategy", StorageLocationSelectorStrategy.class);
Review comment:
Oh sorry, this is a bug. I remember I tested it in my cluster, maybe I missed something then. The code has been updated in the latest commit.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org