Page MenuHomeSoftware Heritage

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

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

Details

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 25762
Build 40267: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 40266: arc lint + arc unit

Event Timeline

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 edited the summary of this revision. (Show Details)
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
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...

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

  • split migration file
  • move new fields at the end of table
  • add specific tests for round-tripping revision and release objects with raw manifest

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

  • Add test_directory_add_with_raw_manifest

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

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

This revision was landed with ongoing or failed builds.Dec 22 2021, 1:29 PM
This revision was automatically updated to reflect the committed changes.

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.