Page MenuHomeSoftware Heritage
Differential D219

Added content_metadata logic to the storage
ClosedPublic
Actions

Authored by moranegg on Jun 27 2017, 4:24 PM.

Details

Reviewers
ardumont
Group Reviewers
Reviewers
Maniphest Tasks
T733: add content_metadata logic to storage
Commits
rDSTOCaf4d3da6c6cf: Merge branch 'content_metadata_logic'
rDSTOC8465e7b92751: Added content_metadata logic to the storage
rDSTOCbb17076fa080: Refactor to keep only content_metadata logic
R65:af4d3da6c6cf: Merge branch 'content_metadata_logic'
R65:bb17076fa080: Refactor to keep only content_metadata logic
R65:8465e7b92751: Added content_metadata logic to the storage
rDSTOaf4d3da6c6cf: Merge branch 'content_metadata_logic'
rDSTObb17076fa080: Refactor to keep only content_metadata logic
rDSTO8465e7b92751: Added content_metadata logic to the storage
Summary

Keeping the indexed content_metadata in the storage with a content_metadata table.
Possibility to add with and without duplications and getting the content_metadata
with tool information.
works only for NPM context also because tool configuration.
References T733

TODO:

  • add json schema in json/ (should we keep CodeMeta schema as is or put PURl to it for the translated_metadata_property?)
Test Plan
  • content_metadata_missing
  • content_metadata_add skipping duplicate
  • content_metadata_add dropping duplicate
  • content_metadata_get
  • db_to_metadata conveter

Diff Detail

Repository
rDSTO Storage manager
Branch
content_metadata_logic
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 924
Build 1227: Software Heritage Python tests
Build 1226: arc lint + arc unit

Event Timeline

moranegg created this revision.Jun 27 2017, 4:24 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptJun 27 2017, 4:24 PM
Harbormaster completed remote builds in B920: Diff 718.Jun 27 2017, 4:25 PM
moranegg updated this revision to Diff 724.Jun 29 2017, 3:06 PM
moranegg added a task: T733: add content_metadata logic to storage.

Added content_metadata logic to the storage

Summary:
- added tables content_metadata, origin_metadata and origin_metadata_history to db schema
- added constraints for content_metadata in indexes.sql
- added functions in swh-func.sql, db.py, storage.py
(references T733 items 1-4)

TODO:
- write constraints for origin_metadata and origin_metadata_history
- add tests
- add tool data in swh-data.sql
- add json schema in json/
Harbormaster completed remote builds in B924: Diff 724.Jun 29 2017, 3:07 PM
moranegg retitled this revision from Added tables content_metadata, origin_metadata and origin_metadata_history to db schema to Added content_metadata logic to the storage.Jun 29 2017, 3:08 PM
moranegg edited the summary of this revision. (Show Details)
moranegg edited reviewers, added: ardumont; removed: Reviewers.
moranegg edited the summary of this revision. (Show Details)Jun 29 2017, 3:11 PM
moranegg added a project: Metadata implementation.
moranegg updated this revision to Diff 727.Jul 7 2017, 3:34 PM
moranegg edited the summary of this revision. (Show Details)

Updating D219: Added content_metadata logic to the storage with tests

Herald added a reviewer: Reviewers. · View Herald TranscriptJul 7 2017, 3:34 PM
Harbormaster completed remote builds in B928: Diff 727.Jul 7 2017, 3:35 PM
moranegg edited the summary of this revision. (Show Details)Jul 7 2017, 3:39 PM
moranegg edited the test plan for this revision. (Show Details)
moranegg removed a reviewer: Reviewers.
ardumont edited edge metadata.Jul 10 2017, 1:43 PM

Except for tidbits (cf. comments inline), this sounds good.

I would prefer that we remove the origin_metadata* schema alteration for another diff though and keep this diff indexer related.

sql/swh-data.sql
880 ↗(On Diff #727)

Please, remove unneeded comments.

sql/swh-schema.sql
515

Can we keep this diff only indexer related?

Personally, I would delay the origin_metadata* table insertion up until we need it (this is not used here :).

swh/storage/storage.py
1609

That's the conversion i told you about that i think is redundant.
Even if metadatas holds more keys than needed, you will only copy the ones mentioned line 1612 below.

As now you have tests, you can remove the conversion and check that tests are still green :)

swh/storage/tests/test_storage.py
3166 ↗(On Diff #727)

metadata did change... :)

moranegg updated this revision to Diff 729.Jul 11 2017, 11:30 AM
moranegg marked 3 inline comments as done.
moranegg edited the summary of this revision. (Show Details)
moranegg edited the test plan for this revision. (Show Details)

update D219 to keep only content_metadata modifications

Herald added a reviewer: Reviewers. · View Herald TranscriptJul 11 2017, 11:30 AM
moranegg added inline comments.Jul 11 2017, 11:30 AM
sql/swh-data.sql
880 ↗(On Diff #727)

it's commented because I don't know how to insert a new tool with same name and version with different config, should I name the tools differently ?

sql/swh-schema.sql
515

yes sure, i wrote it because i wanted comments on the table's design, but it's better to have a new diff for this

swh/storage/storage.py
1609

so if i erase the entire db.copy_to or if i write metadatas as is in the function, the tests don't pass..
but to tell you the truth i'm not sure how to write it differently

swh/storage/tests/test_storage.py
3166 ↗(On Diff #727)

lol :-D

Harbormaster completed remote builds in B930: Diff 729.Jul 11 2017, 11:30 AM
ardumont added inline comments.Jul 11 2017, 9:35 PM
sql/swh-data.sql
880 ↗(On Diff #727)

You should not have any issue with that anymore. I fixed that :)
Simply specify the same name, same version and different configuration as another value.

If you want to be convinced, take a look line 873 and 876.
Those are the same tool with different configuration.

swh/storage/storage.py
1609

so if i erase the entire db.copy_to or if i write metadatas as is in the function, the tests don't pass..

Yes, that could not work.
The copy_to needs to stay, that copies the data we want to insert in a temporary table with sensibly the same schema as the original table. And then the call to db.content_metadata_add_from_temp use that temporary table and its content to insert in the original table.

I was talking about the conversion done within that call (the generator call within parenthesis) can be skipped.
This would give:

db.copy_to(metadatas, 'tmp_content_metadata', ['id', 'translated_metadata', 'indexer_configuration_id'], cur)
moranegg updated this revision to Diff 737.Jul 12 2017, 10:34 AM
moranegg marked 2 inline comments as done.

Updating D219: Added content_metadata logic to the storage

sql/swh-data.sql
880 ↗(On Diff #727)

i'm convinced :-)

but the maven tool replaced the npm tool when i inserted the data, so i had doubts
anyway maven is still not ready at the indexer end, so i'll come back to this when it's ready

swh/storage/storage.py
1609

so strange,
i tried metadatas as is yesterday and it didn't pass the test
now it does.. HOOORAY

Harbormaster completed remote builds in B938: Diff 737.Jul 12 2017, 10:34 AM
ardumont accepted this revision.Jul 12 2017, 6:07 PM
This revision is now accepted and ready to land.Jul 12 2017, 6:07 PM
moranegg updated this revision to Diff 741.Jul 13 2017, 11:55 AM

Merge

Closed by commit rDSTO8465e7b92751: Added content_metadata logic to the storage (authored by moranegg). · Explain WhyJul 13 2017, 11:55 AM
This revision was automatically updated to reflect the committed changes.
Harbormaster completed remote builds in B940: Diff 741.Jul 13 2017, 11:56 AM

Revision Contents
Changeset List

PathSize
sql/
110 lines
14 lines
51 lines
swh/
storage/
29 lines
55 lines

Diff 724

View Options

sql/swh-func.sql

Loading...
View Options

sql/swh-indexes.sql

Loading...
View Options

sql/swh-schema.sql

Loading...
View Options

swh/storage/db.py

Loading...
View Options

swh/storage/storage.py

Loading...
About · Developer information · Legal