Add ResolveContentFutures() #321

ipanova · 2019-03-07T21:21:04Z

No description provided.

pep8speaks · 2019-03-07T21:21:14Z

Hello @ipanova! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-04-02 10:32:52 UTC

asmacdo · 2019-03-08T15:04:22Z

pulp_docker/app/tasks/sync_stages.py

+            pb.increment()
+
+        with ProgressBar(message='Creating Download requests for Tags') as pb:
+            # could use total of len(tag_list) but if schema1 is present done will be < total


Since this is just creating download requests, even schema1 tags will be downloaded the number will be len(tag_list)

asmacdo · 2019-03-08T15:06:32Z

pulp_docker/app/tasks/sync_stages.py

+                pb.increment()
+
+        with ProgressBar(message='Parsing SchemaV2 Tags') as pb:
+            while to_download:


Why did you go with this pattern rather than asyncio.as_completed(to_download)? This seems fine, just curious.

i don't think there was any particular reason, i can change it to as_completed for the sake of readability

dkliban · 2019-03-26T21:14:41Z

@ipanova I fixed some things, but now your branch needs to be rebased.

asmacdo · 2019-03-27T16:56:12Z

pulp_docker/app/tasks/sync_stages.py

@@ -1,11 +1,12 @@
 import json
 import logging
+import asyncio


nit: move to alphabetical order

asmacdo · 2019-03-27T16:58:52Z

pulp_docker/app/tasks/sync_stages.py

+                        raw = content_file.read()
+                    results.artifact_attributes['file'] = results.path
+                    try:
+                        saved_artifact = Artifact(**results.artifact_attributes)


I think this line should be moved out of the try/except.

asmacdo · 2019-03-27T16:59:54Z

pulp_docker/app/tasks/sync_stages.py

+                        pb.increment()
+
+                    else:
+                        continue


Could you add a comment here? It's sort of surprising that no mediatype means schema1.

asmacdo · 2019-03-27T17:01:55Z

pulp_docker/app/tasks/sync_stages.py

-                raise Exception(msg)
-
-    async def create_and_process_tagged_manifest_list(self, tag_dc, manifest_list_data):
+    def create_and_process_tagged_manifest_list(self, tag_dc, manifest_list_data):
        """
        Create a ManifestList and nested ImageManifests from the Tag artifact.


No longer creates nested ImageManifests

asmacdo · 2019-03-27T17:02:23Z

pulp_docker/app/tasks/sync_stages.py

+
+        return list_dc
+
+    def create_and_process_tagged_manifest(self, tag_dc, manifest_data):
        """
        Create a Manifest and nested ManifestBlobs from the Tag artifact.


No longer creates nested ManifestBlobs

Btw, this is what I meant by "process", so maybe the name of the function should be changed too.

asmacdo · 2019-03-27T17:04:38Z

pulp_docker/app/tasks/sync_stages.py

+        try:
+            thru.save()
+        except IntegrityError:
+            pass


Please add a comment here why IntegrityError is ok

asmacdo · 2019-03-27T17:05:34Z

pulp_docker/app/tasks/synchronize.py

+    ResolveContentFutures,
+    QueryExistingArtifacts,
+    QueryExistingContents,
+    ContentSaver,


Alpha order pls

asmacdo · 2019-03-27T17:09:36Z

Took a first pass, nothing but nits, looking great! I'll do a more thorough review with some manual testing after you make the changes we discussed earlier today.

asmacdo

I have not hand tested, but if travis is passing, I am ok to merge. I would rather merge now (even if there are problems) than hand test a lot-- this will help us to build more helpful functional tests and get the current version out to the community.

asmacdo · 2019-04-01T16:18:00Z

pulp_docker/app/tasks/sync_stages.py

-
-
-class TagListStage(Stage):
+class DockerFirstStage(Stage):
    """
    The first stage of a pulp_docker sync pipeline.


Maybe it would be good to add a second paragraph that explains the steps that take place since this is an unusually heavy first stage.

asmacdo · 2019-04-01T16:18:35Z

pulp_docker/app/tasks/sync_stages.py

+        man_dcs = {}
+        total_blobs = []
+
+        with ProgressBar(message='Downloading tag list for the repo', total=1) as pb:


s/for the repo//

asmacdo · 2019-04-01T16:20:10Z

pulp_docker/app/tasks/sync_stages.py

+                to_download.append(downloader.run(extra_data={'headers': V2_ACCEPT_HEADERS}))
+                pb.increment()
+
+        pb1 = ProgressBar(message='Parsed SchemaV2 Tags', state='running')


s/Parsed/Parsing/ to be consistent with the others.

parsed_s2_tags if we plan to do s1. If not parsed_tags.

asmacdo · 2019-04-01T16:20:56Z

pulp_docker/app/tasks/sync_stages.py

+        pb1 = ProgressBar(message='Parsed SchemaV2 Tags', state='running')
+        pb2 = ProgressBar(message='Parsing Manifest List Tags', state='running')
+        pb3 = ProgressBar(message='Parsing Manifests Tags', state='running')
+        global pb4


Why do some but not others need to be global? Perhaps a comment would be helpful.

asmacdo · 2019-04-01T16:22:04Z

pulp_docker/app/tasks/sync_stages.py

+                    await self.put(tag_dc)
+                    pb1.increment()
+                else:
+                    # in case it a schema1 continue to the next tag


Maybe a DEBUG log message would be good here also.

asmacdo · 2019-04-01T16:23:03Z

pulp_docker/app/tasks/sync_stages.py

+        pb3 = ProgressBar(message='Parsing Manifests Tags', state='running')
+        global pb4
+        pb4 = ProgressBar(message='Parsing Blobs', state='running')
+        pb5 = ProgressBar(message='Parsing Manifests', state='running')


I like how you did these progress bars. It nicely separates the workflow we need to follow from the workflow the user would want to see.

asmacdo · 2019-04-01T16:25:12Z

pulp_docker/app/tasks/sync_stages.py

+        pb4 = ProgressBar(message='Parsing Blobs', state='running')
+        pb5 = ProgressBar(message='Parsing Manifests', state='running')
+        while to_download:
+            done, to_download = await asyncio.wait(to_download,


since you use the as_completed loop below, it would be more readable if both loops are the same. (Ignore if this works better for some reason)

i changed this to as_completed, i also moved the saving of the Artifact after we check the mediatype, so we do not save Artifact of schema1

asmacdo · 2019-04-01T16:27:43Z

pulp_docker/app/tasks/sync_stages.py

-    async def create_and_process_tagged_manifest_list(self, tag_dc, manifest_list_data):
-        """
-        Create a ManifestList and nested ImageManifests from the Tag artifact.
+        Create a ManifestList from the Tag artifact.


s/from the Tag artifact//. Since we don't read the artifact here.

asmacdo · 2019-04-01T16:28:29Z

pulp_docker/app/tasks/sync_stages.py

        """
-        Create a pending manifest from manifest data in a ManifestList.
+        Create a Manifest from manifest data in a ManifestList.


More generally, since this data is coming in, it doesn't matter where its from to this code.

i wanted to stress that this manifest is not tagged and when creating it, we rely on some information provided from ML

asmacdo · 2019-04-01T16:30:25Z

pulp_docker/app/tasks/sync_stages.py

+                self.relate_manifest_to_list(dc)
+            elif dc.extra_data.get('blob_relation'):
+                self.relate_blob(dc)
+            elif dc.extra_data.get('config_relation'):


Can a single unit have relations to multiple types of other objects? If so, I think these need to be ifs, not elifs.

my thinking was to create a singe relation type per unit, so elif is correct here

asmacdo · 2019-04-01T16:46:53Z

pulp_docker/app/tasks/sync_stages.py

+                to_download.append(downloader.run(extra_data={'headers': V2_ACCEPT_HEADERS}))
+                pb.increment()
+
+        pb1 = ProgressBar(message='Parsed SchemaV2 Tags', state='running')


parsed_s2_tags if we plan to do s1. If not parsed_tags.

asmacdo · 2019-04-01T16:47:07Z

pulp_docker/app/tasks/sync_stages.py

+                pb.increment()
+
+        pb1 = ProgressBar(message='Parsed SchemaV2 Tags', state='running')
+        pb2 = ProgressBar(message='Parsing Manifest List Tags', state='running')


parsed_manifest_list_tags

asmacdo · 2019-04-01T16:47:20Z

pulp_docker/app/tasks/sync_stages.py

+
+        pb1 = ProgressBar(message='Parsed SchemaV2 Tags', state='running')
+        pb2 = ProgressBar(message='Parsing Manifest List Tags', state='running')
+        pb3 = ProgressBar(message='Parsing Manifests Tags', state='running')


parsed_manifest_tags

closes #4173 closes #4178 https://pulp.plan.io/issues/4173 https://pulp.plan.io/issues/4178

asmacdo reviewed Mar 8, 2019

View reviewed changes

ipanova force-pushed the docker-futures-2 branch 7 times, most recently from b1520c3 to 990817f Compare March 14, 2019 18:39

ipanova force-pushed the docker-futures-2 branch 5 times, most recently from 8f49e99 to 5f88d68 Compare March 26, 2019 16:22

ipanova force-pushed the docker-futures-2 branch 2 times, most recently from a559955 to 0509280 Compare March 27, 2019 10:35

asmacdo reviewed Mar 27, 2019

View reviewed changes

ipanova force-pushed the docker-futures-2 branch 4 times, most recently from e10b2af to 8ec95d3 Compare April 1, 2019 15:45

asmacdo approved these changes Apr 1, 2019

View reviewed changes

asmacdo reviewed Apr 1, 2019

View reviewed changes

Use Futures to handle nested docker content.

568c4a3

closes #4173 closes #4178 https://pulp.plan.io/issues/4173 https://pulp.plan.io/issues/4178

ipanova force-pushed the docker-futures-2 branch from 8ec95d3 to 568c4a3 Compare April 2, 2019 10:32

ipanova merged commit 34a7c62 into pulp:master Apr 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ResolveContentFutures() #321

Add ResolveContentFutures() #321

ipanova commented Mar 7, 2019

pep8speaks commented Mar 7, 2019 •

edited

Loading

asmacdo Mar 8, 2019

asmacdo Mar 8, 2019

ipanova Mar 13, 2019

dkliban commented Mar 26, 2019

asmacdo Mar 27, 2019

asmacdo Mar 27, 2019

asmacdo Mar 27, 2019

asmacdo Mar 27, 2019

asmacdo Mar 27, 2019

asmacdo Mar 27, 2019

asmacdo Mar 27, 2019

asmacdo Mar 27, 2019

asmacdo commented Mar 27, 2019

asmacdo left a comment

asmacdo Apr 1, 2019

asmacdo Apr 1, 2019

asmacdo Apr 1, 2019

asmacdo Apr 1, 2019

asmacdo Apr 1, 2019

asmacdo Apr 1, 2019

asmacdo Apr 1, 2019

asmacdo Apr 1, 2019

ipanova Apr 2, 2019

asmacdo Apr 1, 2019

asmacdo Apr 1, 2019

ipanova Apr 2, 2019

asmacdo Apr 1, 2019

ipanova Apr 2, 2019

asmacdo Apr 1, 2019

asmacdo Apr 1, 2019

asmacdo Apr 1, 2019

Add ResolveContentFutures() #321

Add ResolveContentFutures() #321

Conversation

ipanova commented Mar 7, 2019

pep8speaks commented Mar 7, 2019 • edited Loading

Comment last updated at 2019-04-02 10:32:52 UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkliban commented Mar 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asmacdo commented Mar 27, 2019

asmacdo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Mar 7, 2019 •

edited

Loading