Re: [PR] Make tempStorageDirectory configuration optional and rely on task dir instead (druid)

["cryptoe (via GitHub)" <gi...@apache.org>]

cryptoe commented on code in PR #17015:
URL: https://github.com/apache/druid/pull/17015#discussion_r1800502115


##########
processing/src/main/java/org/apache/druid/storage/StorageConnectorProvider.java:
##########
@@ -20,9 +20,11 @@
 package org.apache.druid.storage;
 
 import com.fasterxml.jackson.annotation.JsonTypeInfo;
-import com.google.inject.Provider;
+
+import java.io.File;
 
 @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type")
-public interface StorageConnectorProvider extends Provider<StorageConnector>
+public interface StorageConnectorProvider
 {
+  StorageConnector createStorageConnector(File tempDir);

Review Comment:
   Could you please add java docs for this method ?
   The current implementation just ignores this tempDir if the property is already set which might confuse the caller. 
   
   Is it possible to not change this interface and have the implementation take  the "tempFIle" as the constructor param. Based on that, we can set the correct tempDir values. 



-- 
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: commits-unsubscribe@druid.apache.org

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