srv-rr-github-token commented on PR #17588:
URL: https://github.com/apache/druid/pull/17588#issuecomment-2566343253
Suggested pull request title: `feat: add LocalTmpStorage interface for
unified temporary storage access`
I appreciate the effort to create a unified interface for managing temporary
storage across all Druid services. The code is well-structured and follows good
Java practices with clear interface definition and implementation.
However, there are a few areas that could be improved to make this change
more robust and maintainable:
1. In `LocalTmpStorage.java`:
- Consider adding documentation about the lifecycle of the temporary
directory (when it's created, when it should be cleaned up)
- Add a method for cleanup/deletion of the temporary directory
- Consider adding checks for directory permissions and existence in the
`DefaultLocalTmpStorageProvider`
2. In service implementations:
- The implementation pattern is inconsistent between CliPeon (using a
custom provider method) and other services (using
DefaultLocalTmpStorageProvider)
- Consider adding proper error handling when temp directory creation
fails in CliPeon's implementation
3. Testing:
- No tests have been added to verify the functionality of the
LocalTmpStorage implementation
- Consider adding unit tests for the DefaultLocalTmpStorageProvider
- Consider adding integration tests to verify temp directory behavior in
different services
4. Documentation:
- Add javadoc comments explaining the purpose and usage of LocalTmpStorage
- Consider adding examples of proper usage in the interface documentation
Please address these points and add appropriate tests before merging. The
overall direction of the change is good, but it needs additional safeguards and
documentation to ensure reliable operation.
_This comment was added by our PR Review Assistant Bot. Please kindly
acknowledge that while we're doing our best to keep these comments up to very
high standards, they may occasionally be incorrect. Suggestions offered by the
Bot are only intended as points for consideration and no statements by this bot
alone can be considered grounds for merging of any pull request. Remember to
seek a review from a human co-worker._
--
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]
