-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Implement a retry mechanism for the FileUploader #2244
Implement a retry mechanism for the FileUploader #2244
Conversation
…reating a decorator
a5b6e20
to
5eafafc
Compare
…he Uploaders and remove `with_retry_policy`
f2c7307
to
a99ec30
Compare
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.
LGTM 👍
match self.wrapped_uploader.upload(filepath).await { | ||
Ok(result) => return Ok(result), | ||
Err(_) if nb_attemps >= self.call_limit => { | ||
return Err(anyhow::anyhow!("Upload retry limit reached")); |
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.
Could you add the total number of attempts in this error message?
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 proposed: "Upload failed after {} attempts"
Do you have a better suggestion ?
* mithril-aggregator from `0.6.20` to `0.6.21`
4363d83
to
26f5996
Compare
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.
LGTM 🚀
let uploader: Box<dyn FileUploader> = Box::new(LocalUploader::new( | ||
SanitizedUrlWithTrailingSlash::parse("http://test.com:8080/base-root/").unwrap(), | ||
&target_dir, | ||
expected_policy.clone(), | ||
TestLogger::stdout(), | ||
)); |
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 use of Box
doesn't seem necessary.
let uploader: Box<dyn FileUploader> = Box::new(LocalUploader::new( | |
SanitizedUrlWithTrailingSlash::parse("http://test.com:8080/base-root/").unwrap(), | |
&target_dir, | |
expected_policy.clone(), | |
TestLogger::stdout(), | |
)); | |
let uploader = LocalUploader::new( | |
SanitizedUrlWithTrailingSlash::parse("http://test.com:8080/base-root/").unwrap(), | |
&target_dir, | |
expected_policy.clone(), | |
TestLogger::stdout(), | |
); |
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.
It's for have a FileUploader
and not a LocalUploader
to be sure that retry_policy
is the function of the trait.
Otherwise, the test pass even if the trait use another function to retrieve policy.
The test name try to explain that (retry_policy_from_file_uploader_trait_should_be_implemented
) but it's probably not enough.
Do you think that we have to add a comment here to explain ?
// Cast into FileUploader to be sure it's the trait function that's being implemented
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.
Oh I get it, that sounds good to me like that 👍. The test name seems sufficient and clear to me.
Content
Extend the FileUploader trait to support a retry mechanism.
This PR includes:
FileUploader
implementationfile uploaders
of theCardanoDatabase
with the default retry values (3 retries and 5 seconds delay)Pre-submit checklist
Issue(s)
Closes #2213