Page MenuHomeSoftware Heritage
Differential D6848

Add columns {,committer_}date_offset to rev/rel and raw_manifest to dir/rev/rel
ClosedPublic
Actions

Authored by vlorentz on Dec 15 2021, 7:04 PM.

Details

Reviewers
olasd
Group Reviewers
Reviewers
Maniphest Tasks
T3752: Store/represent time offsets as strings
T3753: Store original git manifests
Commits
rDSTOf3232e6602f6: Add columns {,committer_}date_offset to rev/rel and raw_manifest to dir/rev/rel
Summary

Depends on D6847

Sorry for the huge diff in postgres code :/

Test Plan

will fail because of the dependency on swh-model

the migration is not tested (as usual)

Diff Detail

Repository
rDSTO Storage manager
Branch
weird-git
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25770
Build 40280: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 40279: arc lint + arc unit

Event Timeline

vlorentz created this revision.Dec 15 2021, 7:04 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptDec 15 2021, 7:04 PM
swh-public-ci added a comment.Dec 15 2021, 7:05 PM

Build has FAILED

Patch application report for D6848 (id=24818)

Rebasing onto c40ceb329f...

Current branch diff-target is up to date.
Changes applied before test
commit 72bdeae016fe6d64b35d28be8032807e23334c74
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Dec 15 19:01:04 2021 +0100

    Add columns {,committer_}date_offset to rev/rel and raw_manifest to dir/rev/rel

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

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 15 2021, 7:05 PM
Harbormaster failed remote builds in B25663: Diff 24818!
vlorentz requested review of this revision.Dec 15 2021, 7:06 PM
vlorentz edited the summary of this revision. (Show Details)
vlorentz edited the test plan for this revision. (Show Details)Dec 15 2021, 7:22 PM
olasd accepted this revision.Dec 20 2021, 12:58 PM
olasd added a subscriber: olasd.

Looks mostly fine to me with a couple of comments, thanks.

I'll try to load a couple of origins in a docker environment and test the migration scripts there.

sql/upgrades/179.sql
160–232

I think this should be split into a separate migration file, we'll have to run it separately anyway.

198
201–202
swh/storage/sql/30-schema.sql
232–235

Please move these fields at the end of the table, so schemas are consistent between updated databases and ones created from scratch. (It makes no functional difference, but it's sometimes useful to be able to diff pg schema dumps)

swh/storage/tests/storage_tests.py
769–774

Do you have a (swh.storage) API extension in mind to retrieve this field? Something like directory_get_manifest?

1157–1159

Shouldn't we expect all the fields be passed back and forth unchanged by swh.storage, even if the raw_manifest is not None (i.e. check that the equality holds in all cases)?

1634–1636

same question here

This revision is now accepted and ready to land.Dec 20 2021, 12:58 PM
vlorentz added inline comments.Dec 20 2021, 1:45 PM
sql/upgrades/179.sql
160–232

yes, that's what I meant

201–202

Hmm I'm not sure that's correct either; I should add NULL handling in _format_offset instead

swh/storage/tests/storage_tests.py
769–774

I don't like it very much because there is no way to paginate :/

But yes, that's the best option I can see

1157–1159

We should, but we can't until https://github.com/pytest-dev/pytest/issues/916 is solved. Imagine the following scenario:

  1. hypothesis calls test_revision_add_get_arbitrary with `Revision(message=b"msg1", raw_manifest=b"manifest", ...)
  2. hypothesis calls test_revision_add_get_arbitrary with `Revision(message=b"msg2", raw_manifest=b"manifest", ...)

Both have exactly the same id (because it was computed from the manifest rather than the fields), and the storage isn't reset between calls to test_revision_add_get_arbitrary by Hypothesis.

So you don't know which revision is in the storage in the end, because it's left undefined by design (it's the one with msg1 when using a pg storage, and the one with msg2 with cass/in-mem)

I could add a new test that doesn't rely on Hypothesis, though...

olasd added inline comments.Dec 22 2021, 1:05 AM
sql/upgrades/179.sql
160–232

I selected the lines for steps 3 and up to suggest moving /these/ to a separate migration file, but I guess that's not clear from phabricator's UI.

swh/storage/tests/storage_tests.py
1157–1159

Ugh, yeah, of course.

I really think having such an explicit test would be useful though (assuming we always insert consistent entries, which should be true...)

vlorentz updated this revision to Diff 24894.Dec 22 2021, 12:45 PM
  • split migration file
  • move new fields at the end of table
  • add specific tests for round-tripping revision and release objects with raw manifest
swh-public-ci added a comment.Dec 22 2021, 12:46 PM

Build has FAILED

Patch application report for D6848 (id=24894)

Rebasing onto f09a54d4f5...

First, rewinding head to replay your work on top of it...
Applying: Add columns {,committer_}date_offset to rev/rel and raw_manifest to dir/rev/rel
Changes applied before test
commit 2bcc32647be7759c26ccdc31952653b6fc90e9a9
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Dec 15 19:01:04 2021 +0100

    Add columns {,committer_}date_offset to rev/rel and raw_manifest to dir/rev/rel

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

Harbormaster failed remote builds in B25761: Diff 24894!Dec 22 2021, 12:46 PM
vlorentz updated this revision to Diff 24895.Dec 22 2021, 12:47 PM
  • Add test_directory_add_with_raw_manifest
vlorentz updated this revision to Diff 24896.Dec 22 2021, 12:48 PM

Bump swh-model requirement

swh-public-ci added a comment.Dec 22 2021, 12:48 PM

Build has FAILED

Patch application report for D6848 (id=24895)

Rebasing onto f09a54d4f5...

First, rewinding head to replay your work on top of it...
Applying: Add columns {,committer_}date_offset to rev/rel and raw_manifest to dir/rev/rel
Changes applied before test
commit 3bd3fb0565086265a2bfade6eaeef660234b8008
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Dec 15 19:01:04 2021 +0100

    Add columns {,committer_}date_offset to rev/rel and raw_manifest to dir/rev/rel

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

Harbormaster failed remote builds in B25762: Diff 24895!Dec 22 2021, 12:48 PM
swh-public-ci added a comment.Dec 22 2021, 12:48 PM

Build has FAILED

Patch application report for D6848 (id=24896)

Rebasing onto f09a54d4f5...

First, rewinding head to replay your work on top of it...
Applying: Add columns {,committer_}date_offset to rev/rel and raw_manifest to dir/rev/rel
Changes applied before test
commit d6dc850589a7a6ae3045d710cce03835ffb3cbbe
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Dec 15 19:01:04 2021 +0100

    Add columns {,committer_}date_offset to rev/rel and raw_manifest to dir/rev/rel

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

Harbormaster failed remote builds in B25763: Diff 24896!Dec 22 2021, 12:48 PM
vlorentz updated this revision to Diff 24903.Dec 22 2021, 1:29 PM

rebase

This revision was landed with ongoing or failed builds.Dec 22 2021, 1:29 PM
Closed by commit rDSTOf3232e6602f6: Add columns {,committer_}date_offset to rev/rel and raw_manifest to dir/rev/rel (authored by vlorentz). · Explain Why
This revision was automatically updated to reflect the committed changes.
vlorentz added a commit: rDSTOf3232e6602f6: Add columns {,committer_}date_offset to rev/rel and raw_manifest to dir/rev/rel.
swh-public-ci added a comment.Dec 22 2021, 1:37 PM

Build is green

Patch application report for D6848 (id=24903)

Rebasing onto f3232e6602...

First, rewinding head to replay your work on top of it...
Fast-forwarded diff-target to base-revision-1516-D6848.
Changes applied before test



See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/1516/ for more details.
Harbormaster completed remote builds in B25770: Diff 24903.Dec 22 2021, 1:37 PM
vlorentz added a task: T3753: Store original git manifests.Jan 7 2022, 1:54 PM
vlorentz added a task: T3752: Store/represent time offsets as strings.
Revision Contents
Changeset List
PathSizeCoverage (All)Coverage (Touched)
2 lines--
sql/
upgrades/
159 lines--
88 lines--
swh/
storage/
7 lines85%
Loading...
cassandra/
3 lines100%
Loading...
13 lines81%
Loading...
4 lines97%
Loading...
postgresql/
31 lines98%
Loading...
7 lines97%
Loading...
2 lines95%
Loading...
sql/
20 lines--
30 lines--
22 lines--
tests/
105 lines99%
Loading...
4 lines100%
Loading...
50 lines100%
Loading...
Diff 24903
View Options
requirements-swh.txt
Loading...
View Options
sql/upgrades/179.sql
Loading...
View Options
sql/upgrades/180.sql
Loading...
View Options
swh/storage/backfill.py
Loading...
View Options
swh/storage/cassandra/model.py
Loading...
View Options
swh/storage/cassandra/schema.py
Loading...
View Options
swh/storage/cassandra/storage.py
Loading...
View Options
swh/storage/postgresql/converters.py
Loading...
View Options
swh/storage/postgresql/db.py
Loading...
View Options
swh/storage/postgresql/storage.py
Loading...
View Options
swh/storage/sql/30-schema.sql
Loading...
View Options
swh/storage/sql/40-funcs.sql
Loading...
View Options
swh/storage/sql/60-indexes.sql
Loading...
View Options
swh/storage/tests/storage_tests.py
Loading...
View Options
swh/storage/tests/test_backfill.py
Loading...
View Options
swh/storage/tests/test_postgresql_converters.py
Loading...
About · Developer information · Legal