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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
