-
Notifications
You must be signed in to change notification settings - Fork 13
Add configurable upload timeout for parcel uploads #17
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
Co-authored-by: amwolff <[email protected]>
amwolff
left a comment
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.
@copilot could you make the timeout not be global but scoped to an individual parcel?
accesslogs/processor.go
Outdated
| opts.UploadingOptions.ShutdownTimeout = time.Minute | ||
| } | ||
| if opts.UploadingOptions.UploadTimeout <= 0 { | ||
| opts.UploadingOptions.UploadTimeout = 5 * time.Minute |
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.
@copilot a 5-minute default is a bit too much. Let's go with a slightly lower value, like 30 seconds.
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.
Changed the default upload timeout from 5 minutes to 30 seconds in commit 9ef9a2c.
- Changed default upload timeout from 5 minutes to 30 seconds - Made timeout scoped to individual parcels instead of global - Added uploadTimeout field to upload struct - Each upload now carries its own timeout value Co-authored-by: amwolff <[email protected]>
Made the timeout scoped to individual parcels in commit 9ef9a2c. Each |
Problem
The access logs uploader was using
context.TODO()when uploading parcels to object storage, which meant there was no timeout for upload operations. This could cause uploads to hang indefinitely if the storage backend became unresponsive or experienced network issues.The issue was identified at:
common/accesslogs/uploader.go
Lines 195 to 197 in b9952c6
Solution
This PR adds a configurable
UploadTimeoutoption that sets a maximum duration for each individual parcel upload operation:UploadTimeoutinOptions.UploadingOptionswith a default of 30 secondscontext.TODO()withcontext.WithTimeout()in the upload loopChanges
uploadTimeoutfield touploadstruct to make timeout scoped to individual parcelsuploadTimeoutfield tosequentialUploaderOptionsandsequentialUploaderstructsOptions.UploadingOptionsto includeUploadTimeout time.Durationwith a default of 30 secondssequentialUploader.run()to use the timeout from each individual uploadqueueUploadandqueueUploadWithoutQueueLimitto pass the timeout when creating uploadsTestUploadTimeoutto verify that uploads properly respect the configured timeoutTesting
All existing tests pass, and a new test specifically validates timeout behavior by:
context.DeadlineExceededExample usage:
Fixes the TODO comment and provides robust timeout handling for production deployments.
Fixes storj/edge#555
Original prompt
Fixes storj/edge#555
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.