Page MenuHomeSoftware Heritage

storages: Refactor journal operations with a dedicated writer collaborator
ClosedPublic

Authored by ardumont on Fri, Feb 7, 2:49 AM.

Details

Summary

Prior to this commit, the code was triplicated across the storage backends. Now
all storages use the same collaborator whose concern is writing to the storage.

Test Plan

tox

Diff Detail

Repository
rDSTO Storage manager
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ardumont created this revision.Fri, Feb 7, 2:49 AM
vlorentz requested changes to this revision.Fri, Feb 7, 11:14 AM

I don't like the name writer, it's less explicit. Maybe keep journal_writer?

Also, you could make the journal writer support model objects, so you don't have to use .to_dict() every time?

swh/storage/in_memory.py
78

is the evolve still needed?

293–296

Filter before the writer...

299–300

... and then you can remove this

(same for other methods below)

This revision now requires changes to proceed.Fri, Feb 7, 11:14 AM
ardumont added inline comments.Fri, Feb 7, 3:25 PM
swh/storage/in_memory.py
78

I don't know, i kept the existing behavior.

299–300

I was aiming at avoiding conflicts from your other incoming diffs...

ardumont added inline comments.Fri, Feb 7, 3:26 PM
swh/storage/in_memory.py
78

And i thought to address this in your incoming 'validation' diff as well.

ardumont updated this revision to Diff 9435.Fri, Feb 7, 3:40 PM
  • storages: Keep journal_writer as collaborator's name
  • Rebase on latest master

Also, you could make the journal writer support model objects, so you don't have to use .to_dict() every time?

hmmm, i wanted to do that after your diffs to decrease the chances of conflicts.

ardumont edited the summary of this revision. (Show Details)Fri, Feb 7, 5:54 PM
ardumont added inline comments.Sat, Feb 8, 10:25 AM
swh/storage/in_memory.py
78

after* your incoming 'validation' diff D2640

Also, yes that attr is needed to trigger the validation check (in the current code context).
In your D2640, it's refactored, so i guess the future rebase i'll do will conflict and i'll adapt it at that moment.

ardumont added a comment.EditedSat, Feb 8, 10:30 AM

Also, you could make the journal writer support model objects, so you don't have to use .to_dict() every time?

hmmm, i wanted to do that after your diffs to decrease the chances of conflicts.

Reflecting a bit again on this, i stand by my statement.
Out storage backend are not yet consistent everywhere in term of dict/model.model.BaseModel objects use.
So that will be a tad annoying to do so right now.

When i do a diff, i try to fix 1 perimeter at a time, not too many at once...
So the code change impacted can be more understandable (also reverted easily if need be).

So here that means, refactor the journal writer related code and that's it.
And i did not change any other logic (so no conversion step, no filtering logic adaptation, etc...).
It's all the more important that i plan to merge this after the validation diff (D2640) take place (which will alter some logic already).
That reduces the surface of code that could conflict.

Cheers,

ardumont updated this revision to Diff 9452.Sat, Feb 8, 10:41 AM
  • writer: Keep the signature to only what's needed
ardumont updated this revision to Diff 9453.EditedSat, Feb 8, 11:26 AM
  • writer: Make the writer able to deal with BaseModel objects

Challenging my own opinion ;)
(i'll be able to tell if i was right or not, i still think i was but let's challenge it)

ardumont updated this revision to Diff 9465.Mon, Feb 10, 5:26 PM
  • Rebase on latest master (includes D2640)
  • Squash commits
ardumont retitled this revision from storages: Refactor journal operations with a dedicated writer collab to storages: Refactor journal operations with a dedicated writer collaborator.Mon, Feb 10, 5:30 PM
ardumont edited the summary of this revision. (Show Details)
vlorentz requested changes to this revision.Mon, Feb 10, 6:14 PM
vlorentz added inline comments.
swh/storage/writer.py
17–29

You don't need this, swh-journal already handles swh-model objects.

This revision now requires changes to proceed.Mon, Feb 10, 6:14 PM
ardumont updated this revision to Diff 9466.Mon, Feb 10, 7:04 PM
ardumont edited the summary of this revision. (Show Details)
  • Add missing types on remaining collaborator method signatures
  • Remove unneeded to_dict conversion as the journal is already swh.model.model.BaseModel aware.
swh/storage/writer.py
17–29

Neat.
Thanks.
Adapted.

ardumont updated this revision to Diff 9467.Mon, Feb 10, 7:16 PM

Remove unnecessary copy instruction (already converting from content object to
a new dict to ease the data key removal)

vlorentz accepted this revision.Tue, Feb 11, 10:25 AM
This revision is now accepted and ready to land.Tue, Feb 11, 10:25 AM
ardumont updated this revision to Diff 9476.Tue, Feb 11, 3:44 PM

Rebase and plug to master branch