-
Notifications
You must be signed in to change notification settings - Fork 123
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
[PULP-276] Add support for prns #3864
Conversation
f0697ee
to
3e948d0
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.
This was copied from the PR adding PRN to pulpcore: pulp/pulpcore#5813.
It did catch the DistributionTree case, where it did not had prn specified.
3e948d0
to
0664071
Compare
I've ran a few experiments after on-demand syncing >10,000 package from centos and here are some conclusions:
[0] Query comparision w/ list of 10,000 packages prns In [54]: timeit(lambda: Content.objects.filter(pk__in=(extract_pk(n) for n in prns)).values("pulp_domain").distinct(), number=10)
Out[55]: 0.17054506699787453
In [56]: timeit(lambda: Content.objects.filter(pk__in=(extract_pk(n) for n in prns),pulp_domain=domain).count(), number=10)
Out[57]: 0.6148349700015387 [1] Very bad performance with extract_pk from pulp_href In [34]: timeit(lambda: [extract_pk(n) for n in prns], number=10)
Out[34]: 0.023607832001289353
In [35]: timeit(lambda: [extract_pk(n) for n in hrefs], number=10)
Out[35]: 22.53728126399801
In [36]: len(prns)
Out[36]: 10000
In [37]: len(hrefs)
Out[37]: 10000 |
Great! If we can do all validation in the view layer with clever queries, that would be greatly preferable to deferring it to the task. If we can reimplement |
34fb058
to
2d58627
Compare
adff5a2
to
6c4fdad
Compare
This is ready to review, but I'm not sure how to handle lowerbounds incompatibility. |
help_text=_( | ||
dedent( | ||
"""\ | ||
A JSON document describing sources, destinations, and content to be copied. |
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.
Content to be copied into the given destinations from the given sources.
|
||
def get_prn(uri): | ||
"""Utility to get prn without having to setup django. | ||
TODO: This is a Copy-paste from pulpcore. Make it public there. |
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.
Remove the TODO
.values("repository__pulp_domain") | ||
.get(pk=resource_pk)["repository__pulp_domain"] | ||
) | ||
else: |
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.
At the very least this function should have a comment stating that it shouldn't be used on anything but RpmRepository
and RepositoryVersion
types. But it would also be good to throw a ValueError
if it's not one of those types.
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.
Approved apart from the last request
* Add cross_domain validation for PRNs on copy API serializer. The new validation doesn't capture the first href/prn that failed anymore, as it relies on querying distinct domains in the bulk of references. Fixes: pulp#3853 fixup: review changes fixup: review changes on class type validation
88b1a10
to
d738251
Compare
Notes: