Page MenuHomeSoftware Heritage
Differential D2771

Make release_add support adding the same object twice in the same call
ClosedPublic
Actions

Authored by douardda on Mar 5 2020, 2:20 PM.

Details

Reviewers
ardumont
vlorentz
olasd
Commits
rDSTOdc1878b9fb50: Make release_add support adding the same object twice in the same call
Summary

This is an edge case, but the mirror infrastructure is apparently hitting it. We
modify the SQL query to be properly idempotent.

Depends on D2773

Test Plan

added new unit test

Diff Detail

Repository
rDSTO Storage manager
Branch
arcpatch-D2771
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11435
Build 17325: Phabricator diff pipeline on jenkinsJenkins · Jenkins console · Jenkins
Build 17324: arc lint + arc unit

Event Timeline

olasd created this revision.Mar 5 2020, 2:20 PM
swh-public-ci added a comment.Mar 5 2020, 2:26 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tox/1019/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tox/1019/console

Harbormaster failed remote builds in B10931: Diff 9863!Mar 5 2020, 2:26 PM
vlorentz added a subscriber: vlorentz.Mar 5 2020, 2:34 PM

I'm guessing you don't want to do the same change for other object types, but you're introducing a subtle difference in the behavior of release_add vs the other ones.
Are you sure you want that?

swh/storage/sql/40-swh-func.sql
685

why the where is needed? the python code already calls release_missing in the same transaction

olasd added a comment.Mar 5 2020, 2:40 PM
In D2771#66211, @vlorentz wrote:

I'm guessing you don't want to do the same change for other object types, but you're introducing a subtle difference in the behavior of release_add vs the other ones.
Are you sure you want that?

Adding contents/skipped_contents already has that behavior; If anything we should align other endpoints to do the same thing. So yes, I'm sure I want that :)

vlorentz added a comment.Mar 5 2020, 3:16 PM
In D2771#66215, @olasd wrote:

So yes, I'm sure I want that :)

I meant, are you sure you don't want to do the same thing for the other endpoints as well?

olasd edited the summary of this revision. (Show Details)Mar 5 2020, 3:55 PM
olasd updated this revision to Diff 9869.Mar 5 2020, 3:55 PM

Rebase on top of D2773

ardumont accepted this revision.Mar 5 2020, 3:58 PM
This revision is now accepted and ready to land.Mar 5 2020, 3:58 PM
swh-public-ci added a comment.Mar 5 2020, 4:07 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tox/1021/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tox/1021/console

Harbormaster failed remote builds in B10937: Diff 9869!Mar 5 2020, 4:07 PM
olasd edited the summary of this revision. (Show Details)Mar 5 2020, 4:10 PM
olasd added a parent revision: D2773: Guarantee the order of results for revision_get and release_get.
swh-public-ci added a comment.Mar 5 2020, 4:17 PM

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

Harbormaster completed remote builds in B10937: Diff 9869.Mar 5 2020, 4:17 PM
vlorentz requested changes to this revision.Mar 6 2020, 1:19 AM

I really think we should either have it for all object types or none at all.

This revision now requires changes to proceed.Mar 6 2020, 1:19 AM
olasd updated this revision to Diff 9932.Mar 9 2020, 4:53 PM

Rebase

swh-public-ci added a comment.Mar 9 2020, 6:51 PM

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

Harbormaster completed remote builds in B11003: Diff 9932.Mar 9 2020, 6:51 PM
douardda added a comment.Mar 12 2020, 1:53 PM
In D2771#66331, @vlorentz wrote:

I really think we should either have it for all object types or none at all.

I agree it should be done everywhere, that said, this fix problems we have right now, so no reason not merge it as is.

douardda accepted this revision.Mar 12 2020, 1:53 PM
vlorentz added inline comments.Mar 13 2020, 11:35 AM
swh/storage/sql/40-swh-func.sql
685

ping?

ardumont added inline comments.Mar 16 2020, 4:06 PM
swh/storage/sql/40-swh-func.sql
685

It'd be great if we could just paste that snippet into an explain...
we can't because the tmp_release does not exist, /me is sad.

olasd added inline comments.Mar 16 2020, 4:10 PM
swh/storage/sql/40-swh-func.sql
685

It's not strictly needed; it's more of a belt + suspenders kind of thing.

vlorentz accepted this revision.Mar 16 2020, 4:20 PM
vlorentz mentioned this in T2316: Align row deduplication of all _add endpoints on release_add.
In D2771#66331, @vlorentz wrote:

I really think we should either have it for all object types or none at all.

I created T2316.

swh/storage/sql/40-swh-func.sql
685

I don't get the point, but whatever...

This revision is now accepted and ready to land.Mar 16 2020, 4:20 PM
olasd planned changes to this revision.Mar 18 2020, 12:48 PM
olasd updated this revision to Diff 10356.Mar 27 2020, 6:49 PM

Noop change to trigger build

This revision is now accepted and ready to land.Mar 27 2020, 6:49 PM
swh-public-ci added a comment.Mar 27 2020, 6:52 PM

Build was aborted

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tox/1117/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tox/1117/console

Harbormaster failed remote builds in B11435: Diff 10356!Mar 27 2020, 6:52 PM
Harbormaster completed remote builds in B11435: Diff 10356.Mar 27 2020, 7:04 PM
Harbormaster failed remote builds in B11435: Diff 10356!Mar 27 2020, 7:14 PM
swh-public-ci added a comment.Mar 27 2020, 7:21 PM

Build was aborted

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/5/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/5/console

swh-public-ci added a comment.Mar 27 2020, 7:26 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/6/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/6/console

swh-public-ci added a comment.Mar 27 2020, 7:37 PM

Build is green
See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/7/ for more details.

Harbormaster completed remote builds in B11435: Diff 10356.Mar 27 2020, 7:37 PM
olasd updated this revision to Diff 10358.Mar 27 2020, 7:42 PM

Actually rebase

olasd added a child revision: D2921: Commit a stupid thing to see whether stacked diffs do something useful.Mar 27 2020, 7:46 PM
swh-public-ci added a comment.Mar 27 2020, 7:46 PM

Build is green
See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/8/ for more details.

Harbormaster completed remote builds in B11437: Diff 10358.Mar 27 2020, 7:46 PM
douardda commandeered this revision.Jun 29 2020, 5:16 PM
douardda edited reviewers, added: olasd; removed: douardda.
douardda updated this revision to Diff 11961.Jun 29 2020, 5:25 PM

rebase and improve a bit the test as well as the cassandra and in_memory backends

douardda updated this revision to Diff 11962.Jun 29 2020, 5:29 PM

kill redundant test

swh-public-ci added a comment.Jun 29 2020, 5:33 PM

Build has FAILED

Patch application report for D2771 (id=11961)

Rebasing onto 10443b8a17...

Current branch diff-target is up to date.
Changes applied before test
commit ca0cb992b9d48ce24ad96b2f659e85a1cc51da88
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon Jun 29 16:23:45 2020 +0200

    Make release_add support adding the same object twice in the same call
    
    This is an edge case, but the mirror infrastructure is apparently hitting it.
    We modify the SQL query to be properly idempotent.
    
    Also ensure in_memory and cassandra backends behave the same.
    
    Note: this revision was mostly written by
          Nicolas Dandrimont <nicolas@dandrimont.eu>.

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/385/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/385/console

Harbormaster failed remote builds in B13178: Diff 11961!Jun 29 2020, 5:33 PM
swh-public-ci added a comment.Jun 29 2020, 5:40 PM

Build is green

Patch application report for D2771 (id=11962)

Rebasing onto 10443b8a17...

Current branch diff-target is up to date.
Changes applied before test
commit dc1878b9fb507a64e4bf8a601efecdd1be8aa2d3
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon Jun 29 16:23:45 2020 +0200

    Make release_add support adding the same object twice in the same call
    
    This is an edge case, but the mirror infrastructure is apparently hitting it.
    We modify the SQL query to be properly idempotent.
    
    Also ensure in_memory and cassandra backends behave the same.
    
    Note: this revision was mostly written by
          Nicolas Dandrimont <nicolas@dandrimont.eu>.

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

Harbormaster completed remote builds in B13179: Diff 11962.Jun 29 2020, 5:40 PM
Closed by commit rDSTOdc1878b9fb50: Make release_add support adding the same object twice in the same call (authored by douardda). · Explain WhyJun 30 2020, 10:07 AM
This revision was automatically updated to reflect the committed changes.
douardda added a commit: rDSTOdc1878b9fb50: Make release_add support adding the same object twice in the same call.
ardumont added a comment.Jun 30 2020, 10:43 AM

Neat

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
sql/
upgrades/
25 lines--
swh/
storage/
sql/
2 lines--
7 lines--
tests/
6 lines98%
Loading...

Diff 10356

View Options

sql/upgrades/147.sql

Loading...
View Options

swh/storage/sql/30-swh-schema.sql

Loading...
View Options

swh/storage/sql/40-swh-func.sql

Loading...
View Options

swh/storage/tests/test_storage.py

Loading...
About · Developer information · Legal