Page MenuHomeSoftware Heritage
Differential D4105

deposit: Adapt loader to send extrinsic raw metadata to the metadata storage
ClosedPublic
Actions

Authored by ardumont on Sep 30 2020, 5:57 PM.

Details

Reviewers
vlorentz
Group Reviewers
Reviewers
Maniphest Tasks
T2649: Modify deposit loader to send metadata from a content-deposit in raw format from to ERMDS
Commits
rDLDBASE777ea4468ad3: deposit: Adapt loader to send extrinsic raw metadata to the metadata storage
Summary

This keeps the metadata written in the revision in the same format as before
though (json dict).

Questions:

  • (storage metadata) Do we keep the raw metadata attached to the revision as it's currently done or do we attach it to the directory instead or both?

-> currently the deposit server writes the raw metadata to the directory (during an update D4013), not on the revision as it cannot.

-> if we need to change it (attach the raw extrinsic metadata to the directory in the metadata storage), that's doable. It's a more involved refactoring though as the package loader does not allow it yet. And this can be done as a next diff.

  • (storage) Do we keep writing the metadata to the revision at all (we could stop now that the loaders are writing to the metadata storage)?

Remark:
Implementation wise, this sends one raw extrinsic metadata column per raw xml received (a deposit can be done in multiple requests, which translates into multiple raw xml files).
The first implementation of this was to join all xml into one concatenated xml. As this meant more handling down the line, when reading, this was changed into this instead.

Related to T2649

Related to D4100
Related to D4101

Test Plan

tox

Diff Detail

Unit TestsFailed
View All

TimeTest
388 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.loader.package.deposit.tests.test_deposit::test_deposit_loading_ok
swh_config = '/tmp/pytest-of-jenkins/pytest-0/test_deposit_loading_ok0/loader.yml' requests_mock_datadir = <requests_mock.mocker.Mocker object at 0x7f48d70f78d0>
381 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.loader.package.deposit.tests.test_deposit::test_deposit_loading_ok_2
swh_config = '/tmp/pytest-of-jenkins/pytest-0/test_deposit_loading_ok_20/loader.yml' requests_mock_datadir = <requests_mock.mocker.Mocker object at 0x7f48d7109be0>
1 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.loader.core.tests.test_converters::test_content_for_storage_data
29 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.loader.core.tests.test_converters::test_content_for_storage_path
1 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.loader.core.tests.test_converters::test_content_for_storage_too_long
View Full Test Results (2 Failed · 144 Passed)

Event Timeline

ardumont created this revision.Sep 30 2020, 5:57 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptSep 30 2020, 5:57 PM
ardumont edited the summary of this revision. (Show Details)Sep 30 2020, 5:58 PM
ardumont added a task: T2649: Modify deposit loader to send metadata from a content-deposit in raw format from to ERMDS.
ardumont updated this revision to Diff 14461.Sep 30 2020, 5:58 PM

Rework commit message to reference the task

swh-public-ci added a comment.Sep 30 2020, 5:59 PM

Build has FAILED

Patch application report for D4105 (id=14460)

Rebasing onto 63e9d3caae...

Current branch diff-target is up to date.
Changes applied before test
commit 928ef5bcdb672fc47b9d7c0217187840b7654813
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 30 17:54:49 2020 +0200

    deposit.loader: Adapt loader to send the extrinsic raw metadata
    
    This almost keeps the revision metadata format as is. It sends the raw
    metadata (xml instead of the parsed json before).

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

Harbormaster failed remote builds in B15737: Diff 14460!Sep 30 2020, 5:59 PM
ardumont planned changes to this revision.Sep 30 2020, 5:59 PM
ardumont edited the summary of this revision. (Show Details)
swh-public-ci added a comment.Sep 30 2020, 6:00 PM

Build has FAILED

Patch application report for D4105 (id=14461)

Rebasing onto 63e9d3caae...

Current branch diff-target is up to date.
Changes applied before test
commit c72ef7e526bec4d6754ef810a84a38b7a4e09aa2
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 30 17:54:49 2020 +0200

    deposit.loader: Adapt loader to send the extrinsic raw metadata
    
    This almost keeps the revision metadata format as is. It sends the raw
    metadata (xml instead of the parsed json before).
    
    Related to T2649

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

Harbormaster failed remote builds in B15738: Diff 14461!Sep 30 2020, 6:00 PM
ardumont updated this revision to Diff 14462.Sep 30 2020, 6:37 PM

Fix tests

swh-public-ci added a comment.Sep 30 2020, 6:39 PM

Build is green

Patch application report for D4105 (id=14462)

Rebasing onto 63e9d3caae...

Current branch diff-target is up to date.
Changes applied before test
commit 95b674783eea1946eaf160b46ae43481c587764a
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 30 17:54:49 2020 +0200

    deposit.loader: Adapt loader to send the extrinsic raw metadata
    
    This almost keeps the revision metadata format as is. It sends the raw
    metadata (xml instead of the parsed json before).
    
    Related to T2649

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

Harbormaster completed remote builds in B15739: Diff 14462.Sep 30 2020, 6:39 PM
ardumont edited the summary of this revision. (Show Details)Sep 30 2020, 6:44 PM
ardumont edited the summary of this revision. (Show Details)
ardumont added inline comments.Sep 30 2020, 6:52 PM
swh/loader/package/deposit/loader.py
70

The all raw xml join i'm talking about in the diff description.
It's either that or sending one RawExtrinsicMetadataCore per xml file below (around line 95) [1]
Like i did for the origin metadata...

I don't know what's best...
I'm now inclined to think that sending multiple raw metadata may be better.

Bottom line, It implies less action when reading.

196

Kept the existing json format as it was before...

210

Here, i did not join the xml files into 1 (as I did for the revision one), shout out if not ok ;)

ardumont updated this revision to Diff 14463.Sep 30 2020, 7:10 PM

Send one raw extrinsic metadata per raw xml received

swh-public-ci added a comment.Sep 30 2020, 7:12 PM

Build is green

Patch application report for D4105 (id=14463)

Rebasing onto 63e9d3caae...

Current branch diff-target is up to date.
Changes applied before test
commit 177f5ec445a573502f53acc077fe3a3807a50102
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 30 17:54:49 2020 +0200

    deposit.loader: Adapt loader to send the extrinsic raw metadata
    
    This almost keeps the revision metadata format as is. It sends the raw
    metadata (xml instead of the parsed json before).
    
    Related to T2649

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

Harbormaster completed remote builds in B15740: Diff 14463.Sep 30 2020, 7:12 PM
ardumont edited the summary of this revision. (Show Details)Sep 30 2020, 7:16 PM
ardumont added inline comments.Sep 30 2020, 7:19 PM
swh/loader/package/deposit/loader.py
70

no longer relevant ^ (this now sends on RawExtrinsicMetadataCore per xml file).

210

The other part was aligned with this.

swh/loader/package/deposit/tests/test_deposit.py
300

cf. my question in the diff, do we still want to write metadata directly in a Revision model object?
My take is no. What do you think?

vlorentz requested changes to this revision.Oct 1 2020, 9:58 AM
vlorentz added a subscriber: vlorentz.

(storage metadata) Do we keep the raw metadata attached to the revision as it's currently done or do we attach it to the directory instead or both?
(storage) Do we keep writing the metadata to the revision at all (we could stop now that the loaders are writing to the metadata storage)?

we should keep writing the metadata to the revision, as JSON for now, just in case we choose to change the schema of metadata again before we migrate. Removing it should be the last step in the migration.

and we can additionally write as dir metadata, but it's outside the scope of this diff

This revision now requires changes to proceed.Oct 1 2020, 9:58 AM
ardumont added a comment.Oct 1 2020, 10:01 AM

we should keep writing the metadata to the revision, as JSON for now, just in case we choose to change the schema of metadata again before we migrate. Removing it should be the last step in the migration.

sounds like a reasonable plan forward, thx.
I'll go and adapt D4100/D4101 towards that.

and we can additionally write as dir metadata, but it's outside the scope of this diff

agreed.

ardumont added a comment.Oct 1 2020, 10:41 AM

sounds like a reasonable plan forward, thx.
I'll go and adapt D4100/D4101 towards that.

Actually standby, i'm no longer convinced this is a good idea, we are discussing in irc...

ardumont mentioned this in D4101: Simplify and document actual information returned to the loader.Oct 1 2020, 11:02 AM
vlorentz added inline comments.Oct 1 2020, 11:30 AM
swh/loader/package/deposit/tests/test_deposit.py
299–301

isn't this the original XML?

ardumont mentioned this in D4100: deposit: Transit raw metadata to the loader to unify with metadata update scenario.Oct 1 2020, 11:52 AM
ardumont added inline comments.Oct 1 2020, 1:11 PM
swh/loader/package/deposit/tests/test_deposit.py
299–301

yes, it is.
It's prior to your requesting change to continue sending the json one.
I've reworked D4100 and D4101 so it continues sending the json metadata.
I'm currently updating the diff with this change.

ardumont updated this revision to Diff 14477.Oct 1 2020, 1:19 PM
  • Adapt according to review, keep the old json format for the metadata written in the revision
  • Rework commit message accordingly
ardumont retitled this revision from deposit.loader: Adapt loader to send the extrinsic raw metadata to deposit: Adapt loader to send extrinsic raw metadata to the metadata storage.Oct 1 2020, 1:19 PM
ardumont edited the summary of this revision. (Show Details)
swh-public-ci added a comment.Oct 1 2020, 1:20 PM

Build is green

Patch application report for D4105 (id=14477)

Rebasing onto 63e9d3caae...

Current branch diff-target is up to date.
Changes applied before test
commit 777ea4468ad3c4e8981151b29f8b70ff7124c4ef
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 30 17:54:49 2020 +0200

    deposit: Adapt loader to send extrinsic raw metadata to the metadata storage
    
    This keeps the metadata written in the revision in the same format as before
    though (json dict).
    
    Related to T2649

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

Harbormaster completed remote builds in B15753: Diff 14477.Oct 1 2020, 1:20 PM
ardumont marked an inline comment as done.Oct 1 2020, 1:20 PM
ardumont added inline comments.
swh/loader/package/deposit/tests/test_deposit.py
299–301

adapted back to keep the old json format now.

vlorentz accepted this revision.Oct 1 2020, 2:02 PM

thx

This revision is now accepted and ready to land.Oct 1 2020, 2:02 PM
Closed by commit rDLDBASE777ea4468ad3: deposit: Adapt loader to send extrinsic raw metadata to the metadata storage (authored by ardumont). · Explain WhyOct 1 2020, 3:28 PM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDLDBASE777ea4468ad3: deposit: Adapt loader to send extrinsic raw metadata to the metadata storage.
vlorentz mentioned this in D4233: migrate_extrinsic_metadata: add support for the new deposit metadata formats introduced in late september..Oct 12 2020, 2:57 PM
vlorentz mentioned this in rDSTOb425b5ca48b8: migrate_extrinsic_metadata: add support for the new deposit metadata formats….Oct 12 2020, 3:57 PM
ardumont mentioned this in D4267: deposit.loader: Avoid unnecessary metadata json transformation.Oct 15 2020, 11:59 AM

Revision Contents
Changeset List

PathSize
swh/
loader/
package/
deposit/
55 lines
tests/
data/
https_deposit.softwareheritage.org/
39 lines
39 lines
109 lines

Diff 14460

View Options

swh/loader/package/deposit/loader.py

Loading...
View Options

swh/loader/package/deposit/tests/data/https_deposit.softwareheritage.org/hello_2.10.json

Loading...
View Options

swh/loader/package/deposit/tests/data/https_deposit.softwareheritage.org/hello_2.11.json

Loading...
View Options

swh/loader/package/deposit/tests/test_deposit.py

Loading...
About · Developer information · Legal