Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

Track remotes. #145

Merged
merged 1 commit into from May 11, 2020
Merged

Track remotes. #145

merged 1 commit into from May 11, 2020

Conversation

ipanova
Copy link
Member

@ipanova ipanova commented Apr 29, 2020

[noissue]

@dralley
Copy link
Contributor

dralley commented May 1, 2020

There should be an easy way to turn it off for testing purposes, even if it's an internal-only flag that is checked.

@ipanova ipanova force-pushed the track-remotes branch 5 times, most recently from c941425 to b1bc315 Compare May 4, 2020 15:46
Comment on lines 123 to 124
# find pre-migrated importers that are specified in plan and are not migrated, meaning that
# whether the remote is missing or it needs to be updated
Copy link
Member

Choose a reason for hiding this comment

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

I think it's ok with out the comment, it completely duplicates the code.

Comment on lines 238 to 236
if pulp2content.pulp3_content is not None and not is_lazy_type:
if pb:
pb.increment()
continue
if pulp2content.pulp3_content is not None and is_lazy_type and not pulp2lazycatalog:
if pb:
pb.increment()
continue
Copy link
Member

Choose a reason for hiding this comment

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

It seems like it doesn't matter whether it's lazy or not. How about this instead of two conditions:

if pulp2content.pulp3_content is not None and not pulp2lazycatalog:
     if pb:
         pb.increment()
     continue

Copy link
Member

Choose a reason for hiding this comment

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

pulp2content.pulp3_content is not None and (not is_lazy_type or is_lazy_type and not pulp2lazycatalog)

pulp_2to3_migration/app/plugin/content.py Outdated Show resolved Hide resolved
pulp_2to3_migration/app/plugin/content.py Outdated Show resolved Hide resolved
Comment on lines 361 to 359
lce.is_migrated = True
dc = DeclarativeContent(content=pulp3content, d_artifacts=[da])
dc.extra_data = future_relations
await self.put(dc)
Pulp2LazyCatalog.objects.bulk_update(objs=pulp2lazycatalog,
fields=['is_migrated'])
else:
Copy link
Member

Choose a reason for hiding this comment

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

Same concern with flipping the flag here.

pulp_2to3_migration/app/pre_migration.py Outdated Show resolved Hide resolved
pulp_2to3_migration/app/pre_migration.py Show resolved Hide resolved
@ipanova ipanova force-pushed the track-remotes branch 3 times, most recently from 9e269de to e8d7681 Compare May 7, 2020 09:39
Copy link
Member

@goosemania goosemania left a comment

Choose a reason for hiding this comment

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

Thanks!
Please find a way to squash 2 conditions we discussed. LGTM for the rest!

@dralley
Copy link
Contributor

dralley commented May 8, 2020

The code looks good, and there's a solid improvement of about 40% in my test w/ 2 repos (Fedora 30 and 31)

This PR:

Intial migration: 798 seconds
Re-migration: 96 seconds

Master branch:

Intial migration: 717 seconds
Re-migration: 164 seconds

The numbers are still pretty rough though. We should look into what the new chokepoint is.

@ipanova ipanova merged commit 8096891 into pulp:master May 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants