Pulp

There's a similar situation in #5731 where added content is actually removed by the finalize step.

I have written two implementations and would like to get feedback on which one we should take (they have different tradeoffs):

1. Normalize the representation during add_content/remove_content (see https://github.com/gmbnomis/pulpcore/commit/00f284b2aa00cfadad5347e7d2ec33df10cf05a1)
2. Add a dedicated normalization function that is called when required (see https://github.com/gmbnomis/pulpcore/commit/baf80d7dd3f2615b8206fb660f9ef88a65a284fb)

Additionally, there is a variant of 2 (see https://github.com/gmbnomis/pulpcore/commit/3a65935d40a4b2788e9e383225a1596325f44c6b):

2a. Add the dedicated normalization function from 2. and adapt the added/removed computation to filter out the non-normalized content

Pros/cons as I see it:

Solution 1:
+ simple implementation
+ added() and removed() always return the correct added and removed content sets
- adds queries to all add_content()/remove_content() calls

Solution 2:
+ In almost all cases, normalization is run only two times for each repository version (before and after finalization). add_content()/remove_content() need additional DB operations in very complex cases only (content is removed/added multiple times)
- added()/removed() may show content that isn't actually added/removed when called on a non-normalized repository version (i.e. during creation of a repository version)
- When we add functionality (e.g. different finalizations for different operations) in the future, we may have to call normalization in additional places

Solution 2a:
+ In almost all cases, normalization is run only a single time for each repository version (before saving). add_content()/remove_content() need additional DB operations in very complex cases only (content is removed/added multiple times)
+ added() and removed() always return the correct added and removed content sets
- added()/removed() have more complicated queries in the common case (but it is still the same number of queries)

I have added/updated option 2a to the comment above which simplifies the trade-off: Before, the main trade-off was between speed and correctness. Now it is mainly between simplicity of implementation and speed. I wouldn't consider the original option 2 anymore.

Thank you for working on some proof-of-concepts for this. I've reviewed both solutions and they both look good to me. With 2/2a, Pulp could still end up with superfluous records in the database if the plugin writer is not careful. Is that correct?

Also, I'm wondering if you have any numbers regarding the trade off in speed.

daviddavis wrote:

Thank you for working on some proof-of-concepts for this. I've reviewed both solutions and they both look good to me. With 2/2a, Pulp could still end up with superfluous records in the database if the plugin writer is not careful. Is that correct?

If the plugin writer uses the context manager, all superfluous records will be removed when exiting the context. Such records may exist during the creation of a version, but in 2a, they are not visible to the plugin writer as added/removed handle them correctly.

Also, I'm wondering if you have any numbers regarding the trade off in speed.

Excellent point, I had a superficial look into this. Short answer: It is very unlikely that 2/2a are worth the effort.

Basically, option 1 creates an additional round trip to the DB for every add_content/remove_content call and I feared that this could be a considerable overhead during sync. That's probably only true if there is not much work to do for a single add_content/remove_content. But sync handles larger batches and that's not the way these calls are used.

In the common case (no superfluous records), the additional round trip (that update/deletes nothing) is negligible when compared to the cost of actually creating/updating RepositoryContent instances.

Applied in changeset pulpcore|c5a887fa6f174f44e1452ed45b2aadde9e15abf2.

Applied in changeset pulpcore|7129d7ef2ea1fdc5255ba658459fce1b34cc6bc0.