New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove tempStorageDirectory and rely on task dir instead #16416
base: master
Are you sure you want to change the base?
Remove tempStorageDirectory and rely on task dir instead #16416
Conversation
} | ||
|
||
@Override | ||
public StorageConnector get() | ||
{ | ||
return new AzureStorageConnector(this, azureStorage); | ||
final File tempDir = injector.getInstance(Key.get(File.class, Names.named(DataSourceTaskIdHolder.TMP_DIR_BINDING))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for this change @adarshsanjeev 👍
Although, it might be worth checking if you can directly JacksonInject
the File object - one way to try it could be to do :
@JacksonInject
@Named(DataSourceTaskIdHolder.TMP_DIR_BINDING)
File tempDir;
Also, could consider adding the JacksonInject
in the constructor itself to keep all the relevant things there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lastly, could be worth having the temp dir instance setup for other processes as well, and also for indexers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried the above initially, but injecting the tempDir as a variable fails (maybe since it is a jackson inject, and that binding has not been added?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it fail for processes which are not peons? if that's the case you can try also marking this Nullable
also, if you have the error available, please feel free to paste it here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above code fails for peons with the error
Guice configuration errors: 1) No implementation for java.io.File annotated with @com.google.inject.name.Named(value="druidTempDirectory") was bound. while locating java.io.File annotated with @com.google.inject.name.Named(value="druidTempDirectory") 1 error
Removes the temporary directory used by durable storage and export, from a user configured value. This PR instead reuses the temporary storage configured for the task.
Pending:
This PR has: