Page MenuHomeSoftware Heritage
Differential D1329

Make snapshot_add take an iterable of snapshot.
ClosedPublic
Actions

Authored by vlorentz on Apr 1 2019, 4:23 PM.

Details

Reviewers
olasd
Group Reviewers
Reviewers
Commits
rDSTOC86308f6431a9: Make snapshot_add take an iterable of snapshot.
R65:86308f6431a9: Make snapshot_add take an iterable of snapshot.
rDSTO86308f6431a9: Make snapshot_add take an iterable of snapshot.
Summary

For uniformity with other _add() endpoints (except origin_visit_add).

Noone uses the new signature of snapshot_add except the tests yet,
so it's the perfect time to make this breaking change.

Depends on D1325.

Diff Detail

Repository
rDSTO Storage manager
Branch
snapshot_add-many
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 5040
Build 6772: tox-on-jenkinsJenkins
Build 6771: arc lint + arc unit

Event Timeline

vlorentz created this revision.Apr 1 2019, 4:23 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptApr 1 2019, 4:23 PM
Harbormaster failed remote builds in B4995: Diff 4230!Apr 1 2019, 4:24 PM
swh-public-ci added a comment.Apr 1 2019, 4:24 PM

Build has FAILED

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

swh-public-ci added a comment.Apr 1 2019, 5:57 PM

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

douardda added a subscriber: douardda.Apr 2 2019, 2:31 PM
douardda added inline comments.
swh/storage/api/client.py
87

wouldn't be acceptable to make this method a bit more flexible and accept snapshots to be a scalar? I mean smthg like:

if isinstance(snapshots, dict):
    snapshots = [snapshots]
vlorentz added inline comments.Apr 2 2019, 4:18 PM
swh/storage/api/client.py
87

Then we should do the same for all other _add methods.

olasd requested changes to this revision.Apr 2 2019, 10:38 PM
olasd added a subscriber: olasd.

If we're doing this, it'd be cleaner to change the SQL logic to actually batch the insertions, rather than doing the loop outside of SQL. Creating temporary tables is fairly expensive and churn on the database catalog is costly (we've been trying to cut it down lately).

You'll need to:

  1. add a snapshot id (sha1_git) column to the temporary table
  2. add all distinct snapshot ids to the snapshot table
  3. add all distinct branch specs to the snapshot_branch table
  4. join all three tables to fill in the snapshot_branches table

Alternatively, you can at least truncate the table rather than dropping it at each loop (but we lose the potential scale benefits of steps 3/4 above).

This revision now requires changes to proceed.Apr 2 2019, 10:38 PM
olasd mentioned this in D1321: swh-storage: Install counter metrics.Apr 2 2019, 11:42 PM
vlorentz added a comment.Apr 3 2019, 10:07 AM

How would you feel about using truncate temporarily, and opening a task to do the right thing later?

olasd added a comment.Apr 3 2019, 10:42 AM
In D1329#28825, @vlorentz wrote:

How would you feel about using truncate temporarily, and opening a task to do the right thing later?

Yeah, that's fine.

vlorentz updated this revision to Diff 4260.Apr 3 2019, 10:54 AM
  • use truncate instead of drop.
vlorentz updated this revision to Diff 4261.Apr 3 2019, 10:55 AM
  • update migration
swh-public-ci added a comment.Apr 3 2019, 10:59 AM

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

Harbormaster completed remote builds in B5028: Diff 4260.Apr 3 2019, 11:00 AM
swh-public-ci added a comment.Apr 3 2019, 11:05 AM

Build has FAILED

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

Harbormaster failed remote builds in B5029: Diff 4261!Apr 3 2019, 11:05 AM
vlorentz updated this revision to Diff 4265.Apr 3 2019, 11:44 AM
  • Add docstring to DirectKafkaWriter.
vlorentz updated this revision to Diff 4267.Apr 3 2019, 11:46 AM
  • undo push to the wrong diff
swh-public-ci added a comment.Apr 3 2019, 11:47 AM

Build has FAILED

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

Harbormaster failed remote builds in B5032: Diff 4265!Apr 3 2019, 11:47 AM
swh-public-ci added a comment.Apr 3 2019, 11:49 AM

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

swh-public-ci added a comment.Apr 3 2019, 11:54 AM

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

Harbormaster completed remote builds in B5034: Diff 4267.Apr 3 2019, 11:54 AM
ardumont mentioned this in D1326: Add DirectKafkaWriter, which collaborates with the Storage to write to Kafka..Apr 3 2019, 2:13 PM
olasd accepted this revision.Apr 3 2019, 2:14 PM

Just one last comment but I think that's good to go.

swh/storage/storage.py
814–815

Can we do the existence check only once (by getting the list of snapshots that don't exist at that point) instead of having it here and in the loop?

This revision is now accepted and ready to land.Apr 3 2019, 2:14 PM
ardumont mentioned this in D1337: core.loader: Adapt according to latest storage api change.Apr 3 2019, 2:19 PM
ardumont added a child revision: D1337: core.loader: Adapt according to latest storage api change.Apr 3 2019, 2:19 PM
vlorentz updated this revision to Diff 4273.Apr 3 2019, 2:47 PM

Don't duplicate calls to db.snapshot_exists.

This revision was landed with ongoing or failed builds.Apr 3 2019, 2:47 PM
Closed by commit rDSTO86308f6431a9: Make snapshot_add take an iterable of snapshot. (authored by vlorentz). · Explain Why
This revision was automatically updated to reflect the committed changes.
Harbormaster failed remote builds in B5040: Diff 4273!Apr 3 2019, 2:47 PM
swh-public-ci added a comment.Apr 3 2019, 2:47 PM

Build has FAILED

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

Revision Contents
Changeset List

PathSize
sql/
upgrades/
46 lines
swh/
storage/
api/
10 lines
37 lines
sql/
1 line
59 lines
tests/
45 lines

Diff 4273

View Options

sql/upgrades/134.sql

Loading...
View Options

swh/storage/api/client.py

Loading...
View Options

swh/storage/in_memory.py

Loading...
View Options

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

Loading...
View Options

swh/storage/storage.py

Loading...
View Options

swh/storage/tests/test_storage.py

Loading...
About · Developer information · Legal