Paths

Table of Contentst

storage.db: Drop db.origin_visit_upsert behavior
ClosedPublic
Actions

Authored by ardumont on Jul 3 2020, 4:34 PM.

Details

Reviewers

vlorentz
douardda

Group Reviewers

Reviewers

Maniphest Tasks

T2310: Make origin visits immutable

Commits

rDSTO348bc7b2204f: storage.db: Drop db.origin_visit_upsert behavior

Summary

The initial desired behavior was to allow creation of origin-visit if they
already had their id set. This is the what's needed for the replayer to actually
work.

But somehow, this left the possibility to update the origin-visit...
This commit fixes it by dropping conflictual origin-visits if any.
In effect, we can no longer overwrite origin-visits (pg-storage wise).

Related to T2310

Test Plan

tox

Diff Detail

Repository

rDSTO Storage manager

Branch

master

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 13333
Build 20373: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 20372: arc lint + arc unit

Event Timeline

ardumont created this revision.Jul 3 2020, 4:34 PM

ardumont edited the summary of this revision. (Show Details)Jul 3 2020, 4:37 PM

Build is green

Patch application report for D3416 (id=12111)

Rebasing onto 248c277445...

First, rewinding head to replay your work on top of it...
Applying: storage.db: Drop db.origin_visit_upsert behavior

Changes applied before test

commit 0b1e32a45ce1eae6dfaa09c1185715b9b8b44299
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Jul 3 16:31:22 2020 +0200

    storage.db: Drop db.origin_visit_upsert behavior
    
    The initial desired behavior was to allow creation of origin-visit if they
    already have their id set. This is what's needed for the replayer.
    
    So now, this will try to add origin-visit and drop conflictual origin-visits.
    
    Related to T2310

See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/406/ for more details.

Harbormaster completed remote builds in B13332: Diff 12111.Jul 3 2020, 4:40 PM

rebase
rework commit message

vlorentz accepted this revision.Jul 3 2020, 4:44 PM

This revision is now accepted and ready to land.Jul 3 2020, 4:44 PM

Build is green

Patch application report for D3416 (id=12112)

Rebasing onto 248c277445...

Current branch diff-target is up to date.

Changes applied before test

commit 348bc7b2204f047ef01cc231aa34fbdbfba98472
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Jul 3 16:31:22 2020 +0200

    storage.db: Drop db.origin_visit_upsert behavior
    
    The initial desired behavior was to allow creation of origin-visit if they
    already had their id set. This is the what's needed for the replayer to actually
    work.
    
    But somehow, this left the possibility to update the origin-visit...
    This commit fixes it by dropping conflictual origin-visits if any.
    In effect, we can no longer overwrite origin-visits (pg-storage wise).
    
    Related to T2310

See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/407/ for more details.

Harbormaster completed remote builds in B13333: Diff 12112.Jul 3 2020, 4:47 PM

Closed by commit rDSTO348bc7b2204f: storage.db: Drop db.origin_visit_upsert behavior (authored by ardumont). · Explain WhyJul 3 2020, 4:49 PM

This revision was automatically updated to reflect the committed changes.

ardumont added a commit: rDSTO348bc7b2204f: storage.db: Drop db.origin_visit_upsert behavior.

The change by itself looks fine, but the cursor manipulation logic is not sparking joy :-)

swh/storage/db.py
487	This call should be using the cursor argument.
490	This needs to be moved above the self.origin_id_get_by_url call
swh/storage/storage.py
825	Missing cursor argument

ardumont mentioned this in T2310: Make origin visits immutable.Jul 3 2020, 4:51 PM

All good points!
Thanks.

ardumont added inline comments.Jul 3 2020, 5:37 PM

swh/storage/db.py
487–488	Funny thing, if i'm calling the method with the "right" `cur`, i got the following error P715 (initial `cur` is passed from storage as well). Without calling that method with cur, it's fine.