[GitHub] [druid] FrankChen021 commented on pull request #10363: fix injection failure of StorageLocationSelectorStrategy objects

[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