FrankChen021 commented on code in PR #19650: URL: https://github.com/apache/druid/pull/19650#discussion_r3520192836
########## indexing-service/src/main/java/org/apache/druid/indexing/worker/shuffle/DeepStorageIntermediaryDataManager.java: ########## @@ -50,9 +52,10 @@ public static String retrieveShuffleDataStoragePath(String supervisorTaskId) } @Inject - public DeepStorageIntermediaryDataManager(DataSegmentPusher dataSegmentPusher) + public DeepStorageIntermediaryDataManager(DataSegmentPusher dataSegmentPusher, DataSegmentKiller dataSegmentKiller) Review Comment: Thanks for checking. I don't think `@Nullable` makes this dependency optional in this path: `DataSegmentKiller` is still bound through `LocalDataStorageDruidModule`'s `PolyBind.createChoice`, and `PolyBind.ConfiggedProvider#get()` throws a `ProvisionException` when `druid.storage.type=s3` cannot be found in the killer map, which still only has `s3_zip`. That happens while Guice resolves the constructor argument, before this constructor can receive `null`, so the MiddleManager deepstore provisioning failure should still reproduce. Reviewed 17 of 17 changed files. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
