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

953665 - added the ability for copy operations to not also copy child un... #181

Merged
merged 3 commits into from Apr 19, 2013

Conversation

mhrivnak
Copy link
Contributor

...its, such as a group copying its RPMs. Also restricted the fetching of existing units to their unit key fields, which reduced RAM use tremendously. Copying a RHEL6 repo went from using about 4.3GB of RAM to < 100MB.

https://bugzilla.redhat.com/show_bug.cgi?id=953665

Please review with: pulp/pulp#443

… units, such as a group copying its RPMs. Also restricted the fetching of existing units to their unit key fields, which reduced RAM use tremendously. Copying a RHEL6 repo went from using about 4.3GB of RAM to < 100MB.
@mhrivnak
Copy link
Contributor Author

I know I haven't added any tests yet for the importer changes. I'll ponder that and address it first thing in the morning. I'm too spent to deal with it at this moment, but the rest of the code would benefit from review.

@@ -285,7 +287,12 @@ def import_units(self, source_repo, dest_repo, import_conduit, config, units=Non
units = import_conduit.get_source_units()
blacklist_units = self._query_blacklist_units(import_conduit, config)
_LOG.info("Importing %s units from %s to %s" % (len(units), source_repo.id, dest_repo.id))
existing_rpm_units_dict = get_existing_units(import_conduit, criteria=UnitAssociationCriteria(type_ids=[TYPE_ID_RPM, TYPE_ID_SRPM]))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

previously we would load ALL of the units in the source repo into RAM even if we never actually used this data. Now we check to see if we intend to use it or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

This certainly looks like an improvement but still concerned that if we determine that we need the existing units, that we're still loading all of the existing units into memory. Perhaps no way around it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm completely with you on that concern. Limiting which fields are loaded (line 292 below) helps a lot with memory use at least. I have a better overall solution in the works in the new importer, but it would take a lot of work to re-implement it here. And considering that this code will all be replaced anyway, my stance is that we shouldn't invest in this any more than we must.

mhrivnak added a commit that referenced this pull request Apr 19, 2013
953665 - added the ability for copy operations to not also copy child un...
@mhrivnak mhrivnak merged commit c079cbd into pulp-2.1 Apr 19, 2013
@mhrivnak mhrivnak deleted the mhrivnak-953665 branch April 19, 2013 18:06
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