Page MenuHomeSoftware Heritage
Differential D3426

Extract revision's extra_header as a top level attribute
ClosedPublic
Actions

Authored by douardda on Jul 6 2020, 10:44 AM.

Details

Reviewers
ardumont
olasd
anlambert
Group Reviewers
Reviewers
Maniphest Tasks
T2423: Extract the `extra_headers` away from `Revision.metadata` into a top-level immutable object
Commits
rDSTO5ab70237d6c3: Extract revision's extra_header as a top level attribute
Summary

Follows swh.model's evolution for the Revision model class.

In Postgresql, store the extra_headers as a bytea[][].

In Cassandra, store them as msgpack dumped blob.

This needs (unpublished) D3389 to pass.

Related to T2423.

Diff Detail

Event Timeline

douardda created this revision.Jul 6 2020, 10:44 AM
Herald added a reviewer: Reviewers. · View Herald TranscriptJul 6 2020, 10:44 AM
swh-public-ci added a comment.Jul 6 2020, 10:45 AM

Build has FAILED

Patch application report for D3426 (id=12127)

Rebasing onto 80108480a6...

Current branch diff-target is up to date.
Changes applied before test
commit 36b6fcc5d6267e084feb415d9de17013055604b6
Author: David Douard <david.douard@sdfa3.org>
Date:   Fri Jul 3 11:45:06 2020 +0200

    Extract revision's extra_header as a top level attribute
    
    Follows swh.model's evolution for the Revision model class.
    
    In Postgresql, store the extra_headers as a bytea[][].
    
    In Cassandra, store them as msgpack dumped blob.

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

Harbormaster failed remote builds in B13348: Diff 12127!Jul 6 2020, 10:45 AM
douardda mentioned this in D3389: Extract the extra_headers from metadata on the Revision model class.Jul 6 2020, 10:49 AM
douardda mentioned this in T2423: Extract the `extra_headers` away from `Revision.metadata` into a top-level immutable object.Jul 6 2020, 1:09 PM
douardda updated this revision to Diff 12144.Jul 6 2020, 2:21 PM

bump dep swh-model>= 0.4.0

swh-public-ci added a comment.Jul 6 2020, 2:26 PM

Build has FAILED

Patch application report for D3426 (id=12144)

Rebasing onto 80108480a6...

Current branch diff-target is up to date.
Changes applied before test
commit 580ebe29ed224cb5c17ffb90e4604797cfc2b3c4
Author: David Douard <david.douard@sdfa3.org>
Date:   Fri Jul 3 11:45:06 2020 +0200

    Extract revision's extra_header as a top level attribute
    
    Follows swh.model's evolution for the Revision model class.
    
    In Postgresql, store the extra_headers as a bytea[][].
    
    In Cassandra, store them as msgpack dumped blob.

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

Harbormaster failed remote builds in B13366: Diff 12144!Jul 6 2020, 2:26 PM
douardda updated this revision to Diff 12147.Jul 6 2020, 2:35 PM

bump deps for swh-journal and swh-core

swh-public-ci added a comment.Jul 6 2020, 2:36 PM

Build has FAILED

Patch application report for D3426 (id=12147)

Rebasing onto 80108480a6...

Current branch diff-target is up to date.
Changes applied before test
commit a2eb6f9c91a5b9a9b3e6acd4cb523647c4e675f6
Author: David Douard <david.douard@sdfa3.org>
Date:   Fri Jul 3 11:45:06 2020 +0200

    Extract revision's extra_header as a top level attribute
    
    Follows swh.model's evolution for the Revision model class.
    
    In Postgresql, store the extra_headers as a bytea[][].
    
    In Cassandra, store them as msgpack dumped blob.

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

Harbormaster failed remote builds in B13369: Diff 12147!Jul 6 2020, 2:36 PM
swh-public-ci added a comment.Jul 6 2020, 2:44 PM

Build is green

Patch application report for D3426 (id=12147)

Rebasing onto 80108480a6...

Current branch diff-target is up to date.
Changes applied before test
commit a2eb6f9c91a5b9a9b3e6acd4cb523647c4e675f6
Author: David Douard <david.douard@sdfa3.org>
Date:   Fri Jul 3 11:45:06 2020 +0200

    Extract revision's extra_header as a top level attribute
    
    Follows swh.model's evolution for the Revision model class.
    
    In Postgresql, store the extra_headers as a bytea[][].
    
    In Cassandra, store them as msgpack dumped blob.

See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/413/ for more details.

swh-public-ci added a comment.Jul 6 2020, 3:30 PM

Build is green

Patch application report for D3426 (id=12147)

Rebasing onto 80108480a6...

Current branch diff-target is up to date.
Changes applied before test
commit a2eb6f9c91a5b9a9b3e6acd4cb523647c4e675f6
Author: David Douard <david.douard@sdfa3.org>
Date:   Fri Jul 3 11:45:06 2020 +0200

    Extract revision's extra_header as a top level attribute
    
    Follows swh.model's evolution for the Revision model class.
    
    In Postgresql, store the extra_headers as a bytea[][].
    
    In Cassandra, store them as msgpack dumped blob.

See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/414/ for more details.

Harbormaster completed remote builds in B13369: Diff 12147.Jul 6 2020, 3:30 PM
olasd added a subscriber: olasd.Jul 6 2020, 3:32 PM

Looks nice.

Few comments:

  • I'm not convinced about the Cassandra storage of these. Have you tried a list <frozen list<blob>> or somesuch?
  • Would it be possible to test the backwards-compatibility feature? That probably means monkey-patching Revision.to_dict() to put extra_headers back in metadata on the way _in_ to the database, but that would make me much more confident in the diff.
douardda added a comment.Jul 6 2020, 4:33 PM
In D3426#84268, @olasd wrote:

Looks nice.

Few comments:

  • I'm not convinced about the Cassandra storage of these. Have you tried a list <frozen list<blob>> or somesuch?

no idea how to do such a thing, let me have a look

  • Would it be possible to test the backwards-compatibility feature? That probably means monkey-patching Revision.to_dict() to put extra_headers back in metadata on the way _in_ to the database, but that would make me much more confident in the diff.

I'll try to.

douardda updated this revision to Diff 12175.Jul 6 2020, 6:55 PM

use frozen<list<list<blob>>> in cassandra's schema for extra_headers

swh-public-ci added a comment.Jul 6 2020, 7:00 PM

Build is green

Patch application report for D3426 (id=12175)

Rebasing onto 80108480a6...

Current branch diff-target is up to date.
Changes applied before test
commit 6674c506ca304ced3da7e5ed137c6aafac25e7f6
Author: David Douard <david.douard@sdfa3.org>
Date:   Fri Jul 3 11:45:06 2020 +0200

    Extract revision's extra_header as a top level attribute
    
    Follows swh.model's evolution for the Revision model class.
    
    In Postgresql, store the extra_headers as a bytea[][].
    
    In Cassandra, store them as frozen<list<list<blob>>>.

See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/415/ for more details.

Harbormaster completed remote builds in B13395: Diff 12175.Jul 6 2020, 7:00 PM
ardumont requested changes to this revision.Jul 7 2020, 10:52 AM
ardumont added a subscriber: ardumont.

For the sake of the missing redefinitions in sql upgrade (and some missing test cases).

Otherwise looks good.

sql/upgrades/158.sql
13

You are missing the redefinition of swh_revision_log, swh_revision_add, swh_content_list_by_object_id.

swh/storage/cassandra/converters.py
49

Use the same conditional as in the prior method above?

if not extra_headers and metadata and "extra_headers" in metadata:
60

That's probably what olasd mentioned already but here we go.
Could you add the necessary use cases test around revision_from_db and revision_to_db?

We can probably just unit-test those...

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

indentation is off.

This revision now requires changes to proceed.Jul 7 2020, 10:52 AM
douardda updated this revision to Diff 12213.Jul 7 2020, 3:20 PM

add a test for "true" bw compat, aka old data in pg still works

(spoil: was not)

swh-public-ci added a comment.Jul 7 2020, 3:26 PM

Build is green

Patch application report for D3426 (id=12213)

Rebasing onto 80108480a6...

Current branch diff-target is up to date.
Changes applied before test
commit a0d93247d39befb4f56370079a61bfccb5844022
Author: David Douard <david.douard@sdfa3.org>
Date:   Fri Jul 3 11:45:06 2020 +0200

    Extract revision's extra_header as a top level attribute
    
    Follows swh.model's evolution for the Revision model class.
    
    In Postgresql, store the extra_headers as a bytea[][].
    Ensure data present in postgres with extra_headers in the metadata field
    are properly supported by the pg-backed storage.
    Get rid of the (now useless) git_headers_to_db() converter function.
    
    In Cassandra, store them as frozen<list<list<blob>>>.

See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/416/ for more details.

Harbormaster completed remote builds in B13433: Diff 12213.Jul 7 2020, 3:26 PM
douardda updated this revision to Diff 12214.Jul 7 2020, 3:35 PM

typos/fixes in 158.sql

ardumont accepted this revision as: ardumont.Jul 7 2020, 3:39 PM

Looks fine to me.

ardumont resigned from this revision.Jul 7 2020, 3:40 PM
ardumont removed a reviewer: ardumont.
ardumont removed a reviewer: Reviewers.
Herald added a reviewer: Reviewers. · View Herald TranscriptJul 7 2020, 3:40 PM
ardumont accepted this revision as: ardumont.Jul 7 2020, 3:40 PM
This revision is now accepted and ready to land.Jul 7 2020, 3:40 PM
swh-public-ci added a comment.Jul 7 2020, 3:42 PM

Build is green

Patch application report for D3426 (id=12214)

Rebasing onto 80108480a6...

Current branch diff-target is up to date.
Changes applied before test
commit aecaec198dfee661086c6c621519f06b717a37a9
Author: David Douard <david.douard@sdfa3.org>
Date:   Fri Jul 3 11:45:06 2020 +0200

    Extract revision's extra_header as a top level attribute
    
    Follows swh.model's evolution for the Revision model class.
    
    In Postgresql, store the extra_headers as a bytea[][].
    Ensure data present in postgres with extra_headers in the metadata field
    are properly supported by the pg-backed storage.
    Get rid of the (now useless) git_headers_to_db() converter function.
    
    In Cassandra, store them as frozen<list<list<blob>>>.

See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/417/ for more details.

Harbormaster completed remote builds in B13434: Diff 12214.Jul 7 2020, 3:42 PM
olasd accepted this revision.Jul 7 2020, 4:02 PM
anlambert requested changes to this revision.Jul 7 2020, 4:16 PM
anlambert added a subscriber: anlambert.
anlambert added inline comments.
swh/storage/sql/30-swh-schema.sql
20

You need to upgrade dbversion here.

This revision now requires changes to proceed.Jul 7 2020, 4:16 PM
anlambert added inline comments.Jul 7 2020, 4:19 PM
sql/upgrades/158.sql
14–39

You can use this instead.

alter type revision_entry add attribute (extra_headers bytea[][]);
douardda added inline comments.Jul 7 2020, 4:45 PM
swh/storage/sql/30-swh-schema.sql
20

jeez, thx

ardumont added inline comments.Jul 7 2020, 4:46 PM
swh/storage/sql/30-swh-schema.sql
20

"given enough eyeballs..."

douardda updated this revision to Diff 12222.Jul 7 2020, 4:50 PM

fix sql schema and simplify the migration script

as reported by anlambert

anlambert accepted this revision.Jul 7 2020, 4:51 PM
This revision is now accepted and ready to land.Jul 7 2020, 4:51 PM
swh-public-ci added a comment.Jul 7 2020, 4:56 PM

Build is green

Patch application report for D3426 (id=12222)

Rebasing onto 80108480a6...

Current branch diff-target is up to date.
Changes applied before test
commit 5ab70237d6c3b8ed357f2ae03589353b8d6e5052
Author: David Douard <david.douard@sdfa3.org>
Date:   Fri Jul 3 11:45:06 2020 +0200

    Extract revision's extra_header as a top level attribute
    
    Follows swh.model's evolution for the Revision model class.
    
    In Postgresql, store the extra_headers as a bytea[][].
    Ensure data present in postgres with extra_headers in the metadata field
    are properly supported by the pg-backed storage.
    Get rid of the (now useless) git_headers_to_db() converter function.
    
    In Cassandra, store them as frozen<list<list<blob>>>.

See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/418/ for more details.

Harbormaster completed remote builds in B13442: Diff 12222.Jul 7 2020, 4:56 PM
Closed by commit rDSTO5ab70237d6c3: Extract revision's extra_header as a top level attribute (authored by douardda). · Explain WhyJul 7 2020, 5:18 PM
This revision was automatically updated to reflect the committed changes.
douardda added a commit: rDSTO5ab70237d6c3: Extract revision's extra_header as a top level attribute.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
3 lines--
2 lines--
4 lines--
1 line--
sql/
upgrades/
13 lines--
swh/
storage/
cassandra/
20 lines95%
Loading...
1 line98%
Loading...
7 lines100%
Loading...
19 lines--
1 line96%
Loading...
sql/
4 lines--
9 lines--
tests/
13 lines100%
Loading...
2 lines100%
Loading...

Diff 12147

View Options

mypy.ini

Loading...
View Options

requirements-swh-journal.txt

Loading...
View Options

requirements-swh.txt

Loading...
View Options

requirements.txt

Loading...
View Options

sql/upgrades/158.sql

Loading...
View Options

swh/storage/cassandra/converters.py

Loading...
View Options

swh/storage/cassandra/cql.py

Loading...
View Options

swh/storage/cassandra/schema.py

Loading...
View Options

swh/storage/converters.py

Loading...
View Options

swh/storage/db.py

Loading...
View Options

swh/storage/sql/30-swh-schema.sql

Loading...
View Options

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

Loading...
View Options

swh/storage/tests/storage_data.py

Loading...
View Options

swh/storage/tests/test_converters.py

Loading...
About · Developer information · Legal