Page MenuHomeSoftware Heritage

Make release_add support adding the same object twice in the same call
AcceptedPublic

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

Details

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
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11437
Build 17330: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 17329: arc lint + arc unit

Event Timeline

olasd created this revision.Mar 5 2020, 2:20 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
686

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

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 :)

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
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

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
686

ping?

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

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
686

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

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
686

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
olasd updated this revision to Diff 10358.Mar 27 2020, 7:42 PM

Actually rebase