Skip to content

Commit dbdcada

Browse files
Copilotfrhuelsz
andauthored
engineering: Reduce verbosity of httpsubfile in high-retry scenarios (#538)
# 🔍 Description Clarity improvements in `crates/trident/src/io_utils/http/subfile.rs` to reduce log noise when servers return empty (0-byte) responses: - **Variable rename**: `received_empty_response` → `previous_response_was_empty` — better captures that the flag reflects the *prior* iteration's state, not the current one. - **Trace message**: `"Successfully populated PartialResponseReader for subfile '{}' at position {} with {} bytes available"` → `"Received a response for subfile '{}' at position {} with {} bytes"` — shorter, avoids implementation-detail noise. - **Silent retry path**: `populate_reader` now retries silently after an empty response (50ms backoff), logging only the first empty-response occurrence to avoid flooding logs with repeated identical messages. - **`silent` parameter**: `new_partial_response_reader` gains a `silent: bool` flag that suppresses the "Requesting HTTP range" trace during silent retries. - **Code comments**: Added an inline comment above the `new_partial_response_reader` call stating that the request should be made silently if we've already tried before and got a zero-length response, and expanded the `new_partial_response_reader` doc comment to describe the `silent` parameter and its effect (when `true`, no logging is produced). # 🤔 Rationale The old name and message were verbose and subtly misleading. In high-retry scenarios (e.g. server returning repeated empty responses), the original code would emit identical trace lines on every attempt. The silent retry path suppresses this noise while still logging the first occurrence so the event is observable. # 📝 Checks - [ ] Check [dev-docs/manual-validation.md](/dev-docs/manual-validation.md) # 📌 Follow-ups TODO: - #0000 # 🗒️ Notes <!-- START COPILOT CODING AGENT TIPS --> --- 🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. [Learn more about Advanced Security.](https://gh.io/cca-advanced-security) --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: frhuelsz <97629030+frhuelsz@users.noreply.github.com>
1 parent 06f9dd0 commit dbdcada

1 file changed

Lines changed: 43 additions & 9 deletions

File tree

crates/trident/src/io_utils/http/subfile.rs

Lines changed: 43 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
use std::{
22
io::{Read, Result as IoResult},
33
ops::Not,
4+
thread,
45
time::Duration,
56
};
67

@@ -165,7 +166,36 @@ impl HttpSubFile {
165166
if self.reader.as_ref().is_none_or(|r| r.exhausted()) {
166167
// Create a new partial response reader for the next range and
167168
// replace any existing reader.
168-
self.reader = Some(self.new_partial_response_reader()?);
169+
let mut previous_response_was_empty = false;
170+
let new_reader = loop {
171+
// If we have already tried and got a zero-length response,
172+
// make the new request silently to avoid log spam.
173+
let reader = self.new_partial_response_reader(previous_response_was_empty)?;
174+
if !reader.exhausted() {
175+
trace!(
176+
"Received a response for subfile '{}' at position {} with {} bytes",
177+
self.url,
178+
self.position,
179+
reader.bytes_remaining,
180+
);
181+
182+
break reader;
183+
}
184+
185+
if !previous_response_was_empty {
186+
previous_response_was_empty = true;
187+
trace!(
188+
"Received empty response when populating reader for subfile '{}' at position {}, retrying silently...",
189+
self.url,
190+
self.position,
191+
);
192+
}
193+
194+
// If we received an empty response, we retry after a short delay.
195+
thread::sleep(Duration::from_millis(50));
196+
};
197+
198+
self.reader = Some(new_reader);
169199
} else {
170200
#[cfg(test)]
171201
trace!(
@@ -181,8 +211,10 @@ impl HttpSubFile {
181211
}
182212

183213
/// Creates a new PartialResponseReader for the current position within
184-
/// the subfile.
185-
fn new_partial_response_reader(&self) -> IoResult<PartialResponseReader> {
214+
/// the subfile. When `silent` is true, the "Requesting HTTP range" trace
215+
/// log is suppressed — used during retries after an empty response to
216+
/// avoid flooding the logs with repeated identical messages.
217+
fn new_partial_response_reader(&self, silent: bool) -> IoResult<PartialResponseReader> {
186218
// Always attempt to read up to the end of the subfile.
187219
let range = HttpRangeRequest::new(
188220
Some(self.start + self.position),
@@ -193,12 +225,14 @@ impl HttpSubFile {
193225

194226
// Perform the HTTP request with retries. This function guarantees that
195227
// the resulting response was successful.
196-
trace!(
197-
"Requesting HTTP range '{}' for subfile at position {}: {}",
198-
range.to_header_value(),
199-
self.position,
200-
self.url,
201-
);
228+
if !silent {
229+
trace!(
230+
"Requesting HTTP range '{}' for subfile at position {}: {}",
231+
range.to_header_value(),
232+
self.position,
233+
self.url,
234+
);
235+
}
202236

203237
let response = super::retriable_request_sender(
204238
|| {

0 commit comments

Comments
 (0)