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

Add rate_limit option to Remote #1091

Merged
merged 1 commit into from Jan 27, 2021
Merged

Add rate_limit option to Remote #1091

merged 1 commit into from Jan 27, 2021

Conversation

fao89
Copy link
Member

@fao89 fao89 commented Jan 27, 2021

https://pulp.plan.io/issues/7965
closes #7965

Please be sure you have read our documentation on creating PRs:
https://docs.pulpproject.org/contributing/pull-request-walkthrough.html

fao89 added a commit to fao89/pulp_ansible that referenced this pull request Jan 27, 2021
bmbouter pushed a commit to pulp/pulp_ansible that referenced this pull request Jan 27, 2021
@@ -0,0 +1 @@
Add ``rate_limit`` option to ``Remote``
Copy link
Member

Choose a reason for hiding this comment

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

Since users don't use this directly I think this changelog should be for plugin writers instead.

@pulpbot
Copy link
Member

pulpbot commented Jan 27, 2021

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

class Migration(migrations.Migration):

dependencies = [
('core', '0054_add_public_key'),
Copy link
Member

Choose a reason for hiding this comment

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

@property
def download_throttler(self):
"""
Return the Throttler which can be used to rate limit downloaders.
Copy link
Member

Choose a reason for hiding this comment

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

This dostring should have a 1-sentence summary with a line separator in between.

Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

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

Thank you!

Comment on lines 306 to 307
Throttler: The instantiated Throttler to be used by
get_downloader()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Throttler: The instantiated Throttler to be used by
get_downloader()
Throttler: The instantiated Throttler to be used by get_downloader()

from asyncio_throttle import Throttler
import django
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
from asyncio_throttle import Throttler
import django
import django
from asyncio_throttle import Throttler

Copy link
Contributor

@daviddavis daviddavis left a comment

Choose a reason for hiding this comment

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

LGTM a couple small suggestions.

@bmbouter
Copy link
Member

I see two approvals!

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