Skip to content
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

Provide a way for the plugin writer to silence warnings from downloaders #1592

Closed
wants to merge 1 commit into from

Conversation

dralley
Copy link
Contributor

@dralley dralley commented Aug 31, 2021

@pulpbot
Copy link
Member

pulpbot commented Aug 31, 2021

Attached issue: https://pulp.plan.io/issues/9317

@mdellweg
Copy link
Member

This does not cover 404s because there is no need to retry. Right?

@dralley
Copy link
Contributor Author

dralley commented Aug 31, 2021

@mdellweg It covers 404s as well. That counts as a "failure" it just happens immediately and doesn't attempt to retry. Basically if quiet=True you won't see anything in the logs about these downloads.

The purpose is we sometimes try to download files that are optional and not guaranteed to be there, so we don't want to log errors in the event that they are not.

Here's a plugin example: pulp/pulp_rpm#2112

@mdellweg
Copy link
Member

That sounds great. Attempting to download metadata that might be there can be quite common.

@@ -234,6 +234,7 @@ async def run(self, extra_data=None):

Args:
extra_data (dict): Extra data passed to the downloader.
quiet (bool): Don't log on backoff or failure events.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to think this over some. This is a leakly abstraction because base has no notion of backoff events, that's a pure HTTPDownloader thing.

Copy link
Contributor Author

@dralley dralley Aug 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use *args and **kwargs instead. I didn't want to do that, because it might tempt passing them on to _run(), which would be improper.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We keep running into this pattern actually. Maybe we should just put *args and **kwargs onto BaseDownloader

@bmbouter
Copy link
Member

How can we integrate this with the existing raise_for_status feature. We've been having plugin writers already using that which silences the 400+ errors already.

@bmbouter
Copy link
Member

Like this actually? Maybe your Pr should replace this usage?

@dralley
Copy link
Contributor Author

dralley commented Aug 31, 2021

@bmbouter I think raising exceptions is necessary for the backoffs to occur properly though, so we can't touch that. Backoff expects exceptions to be thrown when it fails and checks those exceptions to decide what to do next.

It actually covers a separate use case because we don't want to touch the mechanism of the download at all, just avoid logging it.

@bmbouter
Copy link
Member

I think I'm still slow from coming back from paternity. When I read "The purpose is we sometimes try to download files that are optional and not guaranteed to be there, so we don't want to log errors in the event that they are not." I think to myself that is why we made the silence_errors_for_response_status_codes argument here. What you've made here just seems a lot nicer, but there is so much overlap with what we have already I want to somehow not have two things providing piecemeal value. @dralley wdyt?

@dralley
Copy link
Contributor Author

dralley commented Aug 31, 2021

@bmbouter You know, I think this actually does work, but I'm still kind of confused about the mechanism. I'll spend a few minutes digging into the code and then close this PR if it turns out we don't need it.

edit: I think this is probably OK.

@dralley dralley closed this Aug 31, 2021
@dralley dralley deleted the quiet-download branch August 31, 2021 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants