Page MenuHomeSoftware Heritage
Differential D185

storage: open content_update endpoint to permit update on content rows
ClosedPublic
Actions

Authored by ardumont on Mar 2 2017, 10:59 AM.

Details

Reviewers
olasd
zack
Group Reviewers
Reviewers
Commits
rDSTOC96c0a217f1c7: storage: open content_update endpoint
R65:96c0a217f1c7: storage: open content_update endpoint
rDSTO96c0a217f1c7: storage: open content_update endpoint
Summary

Permits to batch update content values (with or without optional new columns).

Limited to contents in the table content. The contents in
skipped_content are not dealt with as we cannot recompute hashes on
those since we don't have them in the objstorage.

Related T692

Note:

  • There are limits to the current implementation as per the todo description in the code (swh.storage.content_update).
  • I have a problem when running the tests for now. Individually, as a set of 2 tests, the new tests run fine. When running all the tests, the one with the new columns hang and i don't know why yet (working on it). This does not block the review though.

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

ardumont created this revision.Mar 2 2017, 10:59 AM
Herald added a reviewer: Reviewers. · View Herald TranscriptMar 2 2017, 10:59 AM
ardumont edited the summary of this revision. (Show Details)Mar 2 2017, 11:08 AM
ardumont added a project: Storage manager.
olasd edited edge metadata.Mar 2 2017, 11:25 AM

I didn't dive deep into the docstring, but at least I gave a stab at the SQL query.

sql/upgrades/101.sql
16

I don't see the point of the two arguments: columns_update has to be a subset of columns_select, and the extra columns in the select argument aren't being used?

31–37

Please use either array_to_string or string concatenation consistently rather than one and the other.

39–45

The function is called update, the main query should be an update.

40

Is this distinct really needed?

swh/storage/storage.py
143

This is inconsistent with the actual function being called

ardumont added inline comments.Mar 2 2017, 11:44 AM
sql/upgrades/101.sql
16

I don't see the point of the two arguments: columns_update has to be a subset of columns_select

I wanted to untangle the complexity in computation of what columns to select and what columns to update.
Debugging those stored procedure is a pain.
Plus, i kept having the wrong result when trying otherwise...

and the extra columns in the select argument aren't being used?

Yes, they are not.
Only the columns_update are to be updated.
The others are effectively dropped.

According to my understanding of the need in T692 (which is the driving need for that api for now) and the 'upsert' documentation.

31–37

Sure. Thanks.

39–45

Right.

40

mmm, i guess not. I'll check for this.

swh/storage/storage.py
143

I'm not sure i get it.

Do you mean it would be inconsistent to call content_update on inexisting content? I would agree with that.

Or maybe,it's the 'for now' which should be dropped altogether?

ardumont added a comment.Mar 2 2017, 1:56 PM

I didn't dive deep into the docstring, but at least I gave a stab at the SQL query.

Well, thanks already :D

ardumont updated this revision to Diff 565.Mar 2 2017, 2:52 PM
  • storage.content_update: Simplify sql update implementation
  • storage.content_update: Simplify sql update implementation
  • storage.content_update: Move altering schema tests in their own class
ardumont updated this revision to Diff 566.Mar 2 2017, 2:54 PM

fixup some bad sql comments commit

  • storage.content_update: Simplify sql update implementation
  • storage.content_update: Move altering schema tests in their own class
ardumont added inline comments.Mar 2 2017, 3:08 PM
swh/storage/storage.py
143

I think the inconsistence you meant was that prior the latest development, the initial sql update function used a copy schema to 'update'. Which indeed could have triggered some new rows if fed with new contents.
I was aware of it, thus the comment and the todo below.

Now, the implementation to update is really an update.
So this should no longer be necessary.
I'll remove this and the todo below.

ardumont updated this revision to Diff 574.Mar 2 2017, 6:47 PM

Rebased to master

  • storage: open content_update endpoint
  • storage.content_update: Provide the select and update columns as parameters
  • storage.content_update: Simplify sql update implementation
  • storage.content_update: Simplify sql update implementation
  • storage.content_update: Move altering schema tests in their own class
  • doc: Update docstring on content_update
ardumont marked 8 inline comments as done.Mar 2 2017, 6:49 PM
ardumont added a comment.EditedMar 3 2017, 10:17 AM

There are limits to the current implementation as per the todo description in the code (swh.storage.content_update).

Done.

I have a problem when running the tests for now. Individually, as a set of 2 tests, the new tests run fine. When running all the tests, the one with the new columns hang and i don't know why yet (working on it). This does not block the review though.

Fixed.

Closed by commit rDSTO96c0a217f1c7: storage: open content_update endpoint (authored by ardumont). · Explain WhyMar 3 2017, 10:26 AM
This revision was automatically updated to reflect the committed changes.
ardumont mentioned this in rDSTO96c0a217f1c7: storage: open content_update endpoint.

Revision Contents
Changeset List

PathSize
sql/
29 lines
2 lines
upgrades/
36 lines
swh/
storage/
api/
4 lines
5 lines
7 lines
32 lines
tests/
59 lines

Diff 575

View Options

sql/swh-func.sql

Loading...
View Options

sql/swh-schema.sql

Loading...
View Options

sql/upgrades/101.sql

Loading...
View Options

swh/storage/api/client.py

Loading...
View Options

swh/storage/api/server.py

Loading...
View Options

swh/storage/db.py

Loading...
View Options

swh/storage/storage.py

Loading...
View Options

swh/storage/tests/test_storage.py

Loading...
About · Developer information · Legal