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

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

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

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

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: hypothesis calls `test_revision_add_get_arbitrary` with `Revision(message=b"msg1", raw_manifest=b"manifest", ...) 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...)

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

Harbormaster failed remote builds in B25761: Diff 24894!Dec 22 2021, 12:46 PM

Add test_directory_add_with_raw_manifest

Bump swh-model requirement

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

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

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.

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.

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

Details

Diff Detail

Event Timeline

Patch application report for D6848 (id=24818)

Changes applied before test

Patch application report for D6848 (id=24894)

Changes applied before test

Patch application report for D6848 (id=24895)

Changes applied before test

Patch application report for D6848 (id=24896)

Changes applied before test

Patch application report for D6848 (id=24903)

Changes applied before test

Revision Contents
Changeset List

Diff 24904

requirements-swh.txt

sql/upgrades/179.sql

sql/upgrades/180.sql

swh/storage/backfill.py

swh/storage/cassandra/model.py

swh/storage/cassandra/schema.py

swh/storage/cassandra/storage.py

swh/storage/postgresql/converters.py

swh/storage/postgresql/db.py

swh/storage/postgresql/storage.py

swh/storage/sql/30-schema.sql

swh/storage/sql/40-funcs.sql

swh/storage/sql/60-indexes.sql

swh/storage/tests/storage_tests.py

swh/storage/tests/test_backfill.py

swh/storage/tests/test_postgresql_converters.py

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

Details

Diff Detail

Event Timeline

Patch application report for D6848 (id=24818)

Changes applied before test

Patch application report for D6848 (id=24894)

Changes applied before test

Patch application report for D6848 (id=24895)

Changes applied before test

Patch application report for D6848 (id=24896)

Changes applied before test

Patch application report for D6848 (id=24903)

Changes applied before test

Revision ContentsChangeset List

Diff 24904

requirements-swh.txt

sql/upgrades/179.sql

sql/upgrades/180.sql

swh/storage/backfill.py

swh/storage/cassandra/model.py

swh/storage/cassandra/schema.py

swh/storage/cassandra/storage.py

swh/storage/postgresql/converters.py

swh/storage/postgresql/db.py

swh/storage/postgresql/storage.py

swh/storage/sql/30-schema.sql

swh/storage/sql/40-funcs.sql

swh/storage/sql/60-indexes.sql

swh/storage/tests/storage_tests.py

swh/storage/tests/test_backfill.py

swh/storage/tests/test_postgresql_converters.py

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

Revision Contents
Changeset List