Details

Reviewers

Group Reviewers

Commits

rDSTOC3457207b22b3: Add a new argument snapshot_id to origin_visit_update, and make all its…
rDSTOC742dd4fa8958: Split Storage.snapshot_add (add the snapshot + update the origin_visit)
R65:3457207b22b3: Add a new argument snapshot_id to origin_visit_update, and make all its…
R65:742dd4fa8958: Split Storage.snapshot_add (add the snapshot + update the origin_visit)
rDSTO742dd4fa8958: Split Storage.snapshot_add (add the snapshot + update the origin_visit)
rDSTO3457207b22b3: Add a new argument snapshot_id to origin_visit_update, and make all its…

Summary

snapshot_add(origin, visit, snap) becomes snapshot_add(snap) +
origin_visit_update(origin, visit, snapshot_id=snap['id'])

Also, all arguments of origin_visit_update become optional; ie. the old value in the DB remains if their value is None. (Side-effect: you can't erase a value unless you provide a new non-None value.)

I added a temporary compatibility layer, so the API change can go painlessly
by updating the clients after the server is deployed.

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

vlorentz created this revision.Mar 28 2019, 1:48 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptMar 28 2019, 1:48 PM

vlorentz edited the summary of this revision. (Show Details)Mar 28 2019, 1:49 PM

vlorentz edited the summary of this revision. (Show Details)

Fix doc of the in-mem storage.

Build is green
See https://jenkins.softwareheritage.org/job/DSTO/job/tox/235/ for more details.

Harbormaster completed remote builds in B4926: Diff 4170.Mar 28 2019, 1:53 PM

Build is green
See https://jenkins.softwareheritage.org/job/DSTO/job/tox/236/ for more details.

Harbormaster completed remote builds in B4927: Diff 4171.Mar 28 2019, 1:57 PM

Fix comment.

Build is green
See https://jenkins.softwareheritage.org/job/DSTO/job/tox/237/ for more details.

Harbormaster completed remote builds in B4928: Diff 4172.Mar 28 2019, 2:09 PM

Rather than jump through hoops and duplicate the visit update code in the legacy codepaths in snapshot_add, I think you could just have them call origin_visit_update after adding the snapshot.

(This changes the behavior a little as adding a snapshot to a non-existent visit will fail after adding the snapshot, but that's not really an issue, and it'll make for some easier to follow code IMO).

This revision now requires changes to proceed.Mar 28 2019, 2:13 PM

In D1314#28207, @olasd wrote:

Rather than jump through hoops and duplicate the visit update code in the legacy codepaths in snapshot_add, I think you could just have them call origin_visit_update after adding the snapshot.

That's already what I do

(This changes the behavior a little as adding a snapshot to a non-existent visit will fail after adding the snapshot, but that's not really an issue, and it'll make for some easier to follow code IMO).

Ok, I'll do that

Remove the compatibility code that checks for visit existence from snapshot_add.

Build is green
See https://jenkins.softwareheritage.org/job/DSTO/job/tox/238/ for more details.

Harbormaster completed remote builds in B4929: Diff 4173.Mar 28 2019, 2:28 PM

Add SQL migration.

Build is green
See https://jenkins.softwareheritage.org/job/DSTO/job/tox/239/ for more details.

Harbormaster completed remote builds in B4930: Diff 4174.Mar 28 2019, 2:34 PM

A few nits left but the rest of the code looks fine to me :)

swh/storage/storage.py
1081–1098	Looks like you're still fetching the visit twice (L1081 and L1091). I think the first if statement can go away and we can just always fetch the full visit metadata (it's only one more join, that we're doing most of the time anyway). That way we: always get the full object we can send to the journal writer can avoid sending an update if the fields haven't been updated (that'd be an "if updates:" before doing both the `write_update` and `db.origin_visit_update`)
swh/storage/tests/test_storage.py
1645 ↗	(On Diff #4174)	Missing a `snapshot_id` kwarg specification here?
1666–1667 ↗	(On Diff #4174)	AFAICT the code path is the same now?