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

ref #1373 - support deferred (Lazy) downloading. #749

Merged
merged 1 commit into from Dec 21, 2015
Merged

ref #1373 - support deferred (Lazy) downloading. #749

merged 1 commit into from Dec 21, 2015

Conversation

jortel
Copy link
Contributor

@jortel jortel commented Dec 18, 2015

https://pulp.plan.io/issues/1425

To start: I tried not to get sucked into making general improvements. But, in some cases, I could not help myself. The refactor of treeinfo.py is a good example. I decided to make that an object mostly to be consistent with the package sync object; and to simply things by not passing so many parameters everywhere. So, most of the large blocks of code removed/added is git's way of dealing with the indentation change so when reviewing - keep in mind that I did not write most of this code.

Some other refactoring was necessary to split (but reuse) downloading and unit creation. It was important to preserve the practice of only adding a unit after files were successfully downloaded. When downloading is deferred (lazy), the downloading is skipped.

In all cases, the catalog is updated.

The unit tests are still a wreck.

@@ -388,11 +370,25 @@ def __init__(self, *args, **kwargs):
self.raw_xml = ''

@property
def relative_path(self): # TODO: what is this used for?
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure this used to be the path where the file was saved in /var/lib/pulp/content/<type>/, but that is no longer the case. This can probably be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The unit.relative_path and unit.download_path is a little helter-skelter in pulp_rpm. These properties along with unit.filename are used inconsistently in the downloading code. I suspect this impl (which I grabbed from the pre-mongoengine model: https://github.com/pulp/pulp_rpm/blob/2.7-dev/plugins/pulp_rpm/plugins/db/models.py#L187) may be attempting to account for downloading the same rpm file name (but different checksum) for two different units into the working directory concurrently.

I even found inconsistency in how these attributes are used when constructing the download URLs.

I think given our improved method for storing unit files in the platform, it may worth straightening this out now.

Note: The code could download files to a unique (local) file name and it will still get stored in the platform correctly. The FileContentUnit.import_content() will do this properly since the FileContentUnit._storage_path is based on the unit ID and will include the file name. Example: end with: zoo.rpm.

@mhrivnak mhrivnak added the LGTM label Dec 21, 2015
@jortel jortel added the Lazy label Dec 21, 2015
@jortel
Copy link
Contributor Author

jortel commented Dec 21, 2015

Tests broken by this PR have been fixed. Still have (SKIP=319)

jortel added a commit that referenced this pull request Dec 21, 2015
ref #1373 - support deferred (Lazy) downloading.
@jortel jortel merged commit 0ad50b6 into pulp:lazy-content Dec 21, 2015
@jortel jortel deleted the issue-1373 branch December 21, 2015 18:25
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

2 participants