On 04/12/2018 04:00 PM, Brian Bouterse wrote:
On Thu, Apr 12, 2018 at 11:53 AM, Jeff Ortel <[email protected]
<mailto:[email protected]>> wrote:
On 04/12/2018 10:01 AM, Brian Bouterse wrote:
On Wed, Apr 11, 2018 at 6:07 PM, Jeff Ortel <[email protected]
<mailto:[email protected]>> wrote:
On 04/11/2018 03:29 PM, Brian Bouterse wrote:
I think we should look into this in the near-term. Changing
an interface on an object used by all plugins will be
significantly easier, earlier.
On Wed, Apr 11, 2018 at 12:25 PM, Jeff Ortel
<[email protected] <mailto:[email protected]>> wrote:
On 04/11/2018 10:59 AM, Brian Bouterse wrote:
On Tue, Apr 10, 2018 at 10:43 AM, Jeff Ortel
<[email protected] <mailto:[email protected]>> wrote:
On 04/06/2018 09:15 AM, Brian Bouterse wrote:
Several plugins have started using the Changesets
including pulp_ansible, pulp_python, pulp_file,
and perhaps others. The Changesets provide several
distinct points of value which are great, but
there are two challenges I want to bring up. I
want to focus only on the problem statements first.
1. There is redundant "differencing" code in all
plugins. The Changeset interface requires the
plugin writer to determine what units need to be
added and those to be removed. This requires all
plugin writers to write the same non-trivial
differencing code over and over. For example, you
can see the same non-trivial differencing code
present in pulp_ansible
<https://github.com/pulp/pulp_ansible/blob/d0eb9d125f9a6cdc82e2807bcad38749967a1245/pulp_ansible/app/tasks/synchronizing.py#L217-L306>,
pulp_file
<https://github.com/pulp/pulp_file/blob/30afa7cce667b57d8fe66d5fc1fe87fd77029210/pulp_file/app/tasks/synchronizing.py#L114-L193>,
and pulp_python
<https://github.com/pulp/pulp_python/blob/066d33990e64b5781c8419b96acaf2acf1982324/pulp_python/app/tasks/sync.py#L172-L223>.
Line-wise, this "differencing" code makes up a
large portion (maybe 50%) of the sync code itself
in each plugin.
Ten lines of trivial set logic hardly seems like a
big deal but any duplication is worth exploring.
It's more than ten lines. Take pulp_ansible for
example. By my count (the linked to section) it's 89
lines, which out of 306 lines of plugin code for sync
is 29% of extra redundant code. The other plugins have
similar numbers. So with those numbers in mind, what do
you think?
I was counting the lines (w/o comments) in find_delta()
based on the linked code. Which functions are you counting?
I was counting the find_delta, build_additions, and
build_removals methods. Regardless of how the lines are
counted, that differencing code is the duplication I'm
talking about. There isn't a way to use the changesets
without duplicating that differencing code in a plugin.
The differencing code is limited to find_delta() and perhaps
build_removals(). Agreed, the line count is less useful than
specifically identifying duplicate code. Outside of
find_delta(), I see similar code (in part because it got
copied from file plugin) but not seeing actual duplication.
Can you be more specific?
Very similar code or identical code, I think it begs the question
why are we having plugin writer's do this at all? What value are
they creating with it? I don't have a reasonable answer to that
question, so the requirement for plugin writer's to write that
code brings me back to the problem statement: "plugin writers
have redundant differencing code when using Changesets". More
info on why it is valuable for the plugin writer to do the
differencing code versus the Changesets would be helpful.
The ChangeSet abstraction (and API) is based on following division
of responsibility:
The plugin (with an understanding of the remote and its content):
- Download metadata.
- Parse metadata
- Based on the metadata:
- determine content to be added to the repository.
- define how artifacts are downloaded.
- construct content
- determine content to be removed to the repository.
Core (without understand of specific remote or its content):
- Provide low level API for plugin to affect the changes it has
determined need to be made to the repository. This is
downloaders, models etc.
- Provide high(er) level API for plugin to affect the changes it
has determined need to be made to the repository. This is the
ChangeSet.
Are you proposing that this is not the correct division?
Yes I believe these problem statements suggest we should adjust the
plugin writer's responsibilities when interacting with the Changesets
in two specific ways. It's not exactly the language you used, but I
believe the following two responsibilities could be moved into the
Changesets entirely:
- determining if any given Artifact or Content unit is already present
in Pulp (aka computing what needs tobe added)
Did you mean /added/ to the repository or /created/ in pulp. Currently,
the plugin determines the content that needs to be added to the
repository. This is modeled using a PendingContent which fully defines
the Content (unit) and its PendingArtifact(s) which are included in the
/additions/. The ChangeSet does determine whether or not any artifacts
need to be downloaded (and downloads them based on policy) and
determines which Content needs to be /created/ vs simply added to the
repository. The plugin blindly assumes that none of the /pending/
content has yet been created pulp. This accomplishes 2 things. 1)
reduces complexity and decision making by the plugin. 2) provides the
ChangeSet with all the information needed to /create/ and /download/ as
needed. The /additions/ represents what the plugin wants to be added to
the repository to synchronize it with the remote repository.
- determining which content units need to be removed (aka computing
the removals)
I don't see how the ChangeSet has enough information to do this. The
plugin can (most likely will) make the decision about what to remove
based on remote metadata, policy and configuration.
^ goals are a restating of the problem statement that plugin writers
are asked to do differencing calculations when the Changesets could
provide that to the plugin writer instead.
So a shorter, simpler problem statement is: "to use the
changesets plugin writers have to do extra work to compute
additions and removals parameters".
This statement ^ is better but still too vague to actually
solve. Can we elaborate on specifically what "to do extra
work" means?
Sure. Removing that vague language is one way to resolve its
vagueness. Here's a revised problem statement: "to use the
changesets plugin writers have to compute additions and removals
parameters". This problem statement would be resolved by a
solution that causes the plugin writer to never have to produce
these parameters and be replaced by an interface that would
require less effort from a plugin writer.
I think it's the plugin's responsibility to determine the
difference. Aside from that: without an understanding of the
metadata and content type, how could the ChangeSet do this? What
might that looks like?
If I'm understanding this correctly, the Changesets already do this
for additions right? Help check my understanding. If a plugin writer
delivers PendingContent and PendingArtifacts to the Changesets as
'additions', the Changesets will recognize them as already downloaded
and not download them right? If this is the case, what is the benefit
of having plugin writers also try to figure out if things should be
downloaded?
As you pointed out, the plugin writer does not need to figure out what
needs to be downloaded.
2. Plugins can't do end-to-end stream processing.
The Changesets themselves do stream processing,
but when you call into changeset.apply_and_drain()
you have to have fully parsed the metadata
already. Currently when fetching all metadata from
Galaxy, pulp_ansible takes about 380 seconds (6+
min). This means that the actual Changeset content
downloading starts 380 seconds later than it
could. At the heart of the problem, the
fetching+parsing of the metadata is not part of
the stream processing.
The additions/removals can be any interable (like
generator) and by using ChangeSet.apply() and
iterating the returned object, the pluign can "turn
the crank" while downloading and processing the
metadata. The ChangeSet.apply_and_drain() is just a
convenience method. I don't see how this is a
limitation of the ChangeSet.
That is new info for me (and maybe everyone). OK so
Changesets have two interfaces. apply() and
apply_and_drain(). Why do we have two interfaces when
apply() can support all existing use cases (that I know
of) and do end-to-end stream processing but
apply_and_drain() cannot? I see all of our examples
(and all of our new plugins) using apply_and_drain().
The ChangeSet.apply() was how I designed (and
documented) it. Not sure when/who added the
apply_and_drain(). +1 for removing it.
I read through the changeset docs. I think this stream
processing thing is still a problem but perhaps in how we're
presenting the Changeset with it's arguments. I don't think
apply() versus apply_and_drain() are at all related.
Regardless of if you are using apply() or apply_and_drain(),
the Changeset requires an 'additions' and 'removals'
arguments. This sends a clear message to the plugin writer
that they need to compute additions and removals. They will
fetch the metadata to compute these which is mostly how the
changeset documentation reads. To know that they could
present a generator that would correctly allow the metdata
from inside the Changeset is I feel as non-obvious. I want
the high-performing implementation to be the obvious one.
So what about a problem statement like this: "Changesets are
presented such that when you call into them you should
already have fetched the metadata"?
I'm not sure what is meant by "presented". If this means
that we should provide an example of how the ChangeSet can be
used by plugins (with large metadata) in such a way that does
not require downloading all the metadata first - that sounds
like a good idea.
Cool so this is transitioning to ideas for resolution. The
solution to add documentation on how to do this with the existing
interface is one option. My concern with adding additional docs
on how to use the current interface better is that if users
choose to follow the existing docs then they will have the stream
processing problem once again. To me, this suggests that this new
example should actually replace the existing documentation.
Seems like both example would be useful. I'm not convinced that
all plugins would benefit from this. For example: the File plugin
manifest is small and would likely not benefit from the extra
complexity. For complicated plugins (like RPM), can differencing
decision be made before analyzing the entire metadata (eg:
primary.xml)? Also, it's not clear to me how this would work
using the Downloader. Are you suggesting that the plugin would
parse/process metadata files while they're being downloaded?
Perhaps a better understanding of the flow to be supported would
help me understand this.
Yes I am suggesting just that: that the Changesets could facilitate
parse/processing metadata files while actual content named in those
files is also being downloaded. I have a straightforward idea on how
to achieve this. It's short and easy enough to write up (no code), but
I want to make sure I'm not moving beyond the problem statement
without others. Is there more we want to do on these problem
statements, or would answering a bit about one way it could work be
helpful?
The /additions/ can be (and usually is) a generator. The generator can
yield based on metadata as it is downloaded and digested. In this way,
the ChangeSet already facilitates this.
Just to state my expectations: Moving beyond the problem statement I
don't consider to be a commitment to solve it; just an agreement on
what we're solving as we discuss various resolutions. Problem
statements can also always be revisited. Either way forward is fine w/
me, just let me know how we should continue
So far, I'm not convinced that any specific problems/deficiencies have
been identified. That said, you seem to have a different abstraction in
mind. I would be interested in reviewing it and how it would be used by
plugin writers. It may help illustrate the gains that you are envisioning.
Do you see the same challenges I do? Are these the
right problem statements? I think with clear
problem statements a solution will be easy to see
and agree on.
I'm not convinced that these are actual
problems/challenges that need to be addressed in
the near term.
Thanks!
Brian
_______________________________________________
Pulp-dev mailing list
[email protected] <mailto:[email protected]>
https://www.redhat.com/mailman/listinfo/pulp-dev
<https://www.redhat.com/mailman/listinfo/pulp-dev>
_______________________________________________
Pulp-dev mailing list
[email protected] <mailto:[email protected]>
https://www.redhat.com/mailman/listinfo/pulp-dev
<https://www.redhat.com/mailman/listinfo/pulp-dev>
_______________________________________________
Pulp-dev mailing list
[email protected] <mailto:[email protected]>
https://www.redhat.com/mailman/listinfo/pulp-dev
<https://www.redhat.com/mailman/listinfo/pulp-dev>
_______________________________________________
Pulp-dev mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/pulp-dev
