Pulp

Does it make any sense to implement this as a save() or post_save() callback on RepositoryContentUnit itself? New saves vs updates could be handled accordingly. We could also hook delete(). We may loose a little bit of performance doing it unit by unit, but the update can be done efficiently such as with the update_unit_count() method. This would take us closer to an object oriented design.

Pulp wrote:

Does it make any sense to implement this as a save() or post_save()
callback on RepositoryContentUnit itself? New saves vs updates could be
handled accordingly. We could also hook delete(). We may loose a little
bit of performance doing it unit by unit, but the update can be done
efficiently such as with the update_unit_count() method. This would take
us closer to an object oriented design.

Unfortunately, save()/delete() are only called when individual units are
saved/deleted, not when collection-wide update queries are called. I'm
not sure what kinds of queries are performed in all cases, but if any
bulk-insert queries are used this wouldn't work. In general, it's not
great to rely on save and delete in this way for models with Mongoengine
(or Django either).

--
Randy Barlow
irc: bowlofeggs

rbarlow wrote:

Unfortunately, save()/delete() are only called when individual units are
saved/deleted, not when collection-wide update queries are called. I'm
not sure what kinds of queries are performed in all cases, but if any
bulk-insert queries are used this wouldn't work. In general, it's not
great to rely on save and delete in this way for models with Mongoengine
(or Django either).

--
Randy Barlow
irc: bowlofeggs

To clarify, I'm proposing these save()/delete() hooks be done on the RepositoryContentUnit model which effectively is the association of units to a repo. I can't think of any bulk operation that wouldn't operate on RepositoryContentUnit and cause save()/delete() operations. The benefit of this approach is reusability. Future code could work with the RepositoryContentUnit model directly and receive the correct behaviors instead of using import_unit, remove_unit, delete_unit.

I want to check in on my understanding here. Is it correct that the content counts of types for a repo are stored on the repo for performance reasons only? Otherwise the RepositoryContentUnit collection itself would provide these counts with queries, right?

Also aren't import_unit and remove_unit the only two places where counts are modified? Shouldn't sync() associate and disassociate units using import_unit or remove_unit? Similarly for upload_unit, should that just call import_unit at some point? FYI, I'm not very familiar with the plugin API.

bmbouter wrote:

To clarify, I'm proposing these save()/delete() hooks be done on the RepositoryContentUnit model which effectively is the association of units to a repo. I can't think of any bulk operation that wouldn't operate on RepositoryContentUnit and cause save()/delete() operations. The benefit of this approach is reusability. Future code could work with the RepositoryContentUnit model directly and receive the correct behaviors instead of using import_unit, remove_unit, delete_unit.

There's a downside too - instead of one query to update the count after a sync of 10,000 units, there will be 10,000 queries. I think that will be noticeable, especially with the new lazy feature.

I want to check in on my understanding here. Is it correct that the content counts of types for a repo are stored on the repo for performance reasons only? Otherwise the RepositoryContentUnit collection itself would provide these counts with queries, right?

Yes, it's a denormalization. We used to do a query, and I believe mhrivnak made the change to store it this way for rapid access.

Also aren't import_unit and remove_unit the only two places where counts are modified? Shouldn't sync() associate and disassociate units using import_unit or remove_unit? Similarly for upload_unit, should that just call import_unit at some point? FYI, I'm not very familiar with the plugin API.

No, import_units(), upload(), sync(), and remove_units() are all methods on an Importer, and each of them can perform bulk actions.

For now, I think we should stick to the mechanism we have. This issue is just about moving the call to that mechanism from one place to another, which is a big improvement, and very simple.

Changing how we do it is a worthy discussion, but a separate and more complex issue.

mhrivnak wrote:

For now, I think we should stick to the mechanism we have. This issue is just about moving the call to that mechanism from one place to another, which is a big improvement, and very simple.

Changing how we do it is a worthy discussion, but a separate and more complex issue.

Agreed. Let's keep this to fixing just those specific calls. I'm convinced that the save()/delete() design won't work because of the performance reasons rbarlow mentioned.