-
Notifications
You must be signed in to change notification settings - Fork 15
feature: Resilient Http File reader #458
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
base: main
Are you sure you want to change the base?
Conversation
…sz/direct-stream/subfile-reader
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.
Pull request overview
This PR introduces a resilient HTTP file reader by creating a new HttpSubFile struct that handles reading byte ranges from HTTP resources with automatic retry and recovery from transient network errors. The changes refactor HTTP-related code into a dedicated submodule and migrate from directly using HTTP responses to using the new HttpSubFile abstraction.
Changes:
- Introduces
HttpSubFilestruct implementingReadwith transparent multi-request handling and network error recovery - Refactors HTTP code into dedicated
httpsubmodule withfile,range, andsubfilecomponents - Migrates
HttpFilemethods to returnHttpSubFileobjects instead of raw HTTP responses
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/trident/src/io_utils/mod.rs | Updates module visibility from public http_file to private http submodule |
| crates/trident/src/io_utils/http/subfile.rs | Implements new HttpSubFile struct with resilient reading logic and comprehensive tests |
| crates/trident/src/io_utils/http/range.rs | Adds HttpRangeRequest helper struct for HTTP range header generation |
| crates/trident/src/io_utils/http/mod.rs | Centralizes common HTTP utilities including error conversion and retry logic |
| crates/trident/src/io_utils/http/file.rs | Refactors HttpFile to use HttpSubFile and migrates shared code to parent module |
| crates/trident/src/io_utils/file_reader.rs | Updates imports and removes unwrap calls from section_reader usage |
Comments suppressed due to low confidence (1)
crates/trident/src/io_utils/http/file.rs:344
- The
read_to_endimplementation incorrectly usesbuf.len()which represents the current buffer size, not the remaining bytes to read. This should beself.size - self.positionto read from current position to the end of the file. The current implementation will only attempt to read as many bytes as the buffer currently contains, not all remaining bytes in the file.
|
/AzurePipelines run [GITHUB]-trident-pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
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.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (5)
crates/trident/src/io_utils/http/file.rs:105
- The warning message uses string interpolation for the constant but the constant is not interpolated correctly. This should use
{:?}or concatenation. Currently it will print the debug representation of the constant type. Change to:warn!(\"OCI server does not provide '{}' header, continuing anyway\", ACCEPT_RANGES);
crates/trident/src/io_utils/http/file.rs:108 - Similar to the warning above, the constant is used in string interpolation syntax but won't be interpolated properly. Use standard format argument:
format!(\"Server does not support range requests: '{}' header was not provided\", ACCEPT_RANGES)
crates/trident/src/io_utils/http/file.rs:114 - The constant won't be interpolated correctly in this format string. Use:
format!(\"Could not parse '{}': {e}\", ACCEPT_RANGES)
crates/trident/src/io_utils/http/file.rs:121 - The constant won't be interpolated correctly. Use:
format!(\"Server does not support range requests: '{}': none\", ACCEPT_RANGES)
crates/trident/src/io_utils/http/file.rs:173 - The debug message on line 172 references
docker_config_path.display()but the path construction happens on line 169. The message on line 171-173 would be clearer if it said 'Docker config file does not exist' since we've already determined the path. Consider revising to:debug!(\"Docker config file does not exist at '{}'\", docker_config_path.display());
…rect-stream/subfile-reader
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.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
crates/trident/src/io_utils/http/file.rs:346
- Missing optimization in read_to_end implementation. The read_to_end method creates a section_reader but doesn't call with_end_is_parent_eof() like the complete_reader() method does when reading to the end of the file. When self.position is 0 (reading the entire file from the start), consider using complete_reader() instead, or call with_end_is_parent_eof() on the subfile when reading to the end of the file to avoid unnecessary Range header on the last segment.
|
/AzurePipelines run [GITHUB]-trident-pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
|
||
| /// Get authentication credentials for accessing registry. Unless "dangerous-options" flag is | ||
| /// enabled, will default to anonymous access. | ||
| fn get_auth(_img_ref: &Reference) -> RegistryAuth { |
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.
nit: it would be nice if get_auth were something like get_docker_auth
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.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
|
/AzurePipelines run [GITHUB]-trident-pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
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.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
🔍 Description
HttpSubFilestruct that is now the preferred way to read from an HTTP file. Instead of performing a request and returning the response,HttpFilenow returns anHttpSubFileobject which will internally handle all complexities of reading a file via HTTP. This includes transparently handling multiple requests when the server cannot provide a full range or when a network error interrupts the read operation.Depends on #461 because I've been using that for testing.