Page MenuHomeSoftware Heritage
Differential D93

Make the archiver use content_archive.num_present
ClosedPublic
Actions

Authored by qcampos on Aug 5 2016, 4:27 PM.

Details

Reviewers
olasd
zack
Group Reviewers
Reviewers
Maniphest Tasks
T512: Make archiver have a more symmetrical behavior treating storages as potential sources & destinations at the same time
Commits
rDSTOC48f4c05b66b7: Make the archiver use content_archive.num_present
R65:48f4c05b66b7: Make the archiver use content_archive.num_present
rDSTO48f4c05b66b7: Make the archiver use content_archive.num_present
Summary

content_archive.num_present act like a cache for the number of copies
where the content is present. This allow to make a much faster request
when retrieving the contents that have fewer copies than expected.

This diff works with the commit 9fdbadd0c48c4e76b0be2f9a32a09ef41a38d3d4,
and they follow a previous discussion in D86 about this optimization.

Diff Detail

Repository
rDSTO Storage manager
Branch
T512
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 374
Build 553: Software Heritage Python tests
Build 552: arc lint + arc unit

Event Timeline

qcampos updated this revision to Diff 312.Aug 5 2016, 4:27 PM
qcampos added a task: T512: Make archiver have a more symmetrical behavior treating storages as potential sources & destinations at the same time.
qcampos retitled this revision from to Make the archiver use content_archive.num_present.
qcampos updated this object.
qcampos edited the test plan for this revision. (Show Details)
qcampos added a reviewer: olasd.
Herald added a reviewer: Reviewers. · View Herald TranscriptAug 5 2016, 4:27 PM
olasd edited edge metadata.Aug 5 2016, 7:35 PM

Note that in the current deployment the column is not indexed and we should make sure it is before deploying this code.

zack requested changes to this revision.Aug 10 2016, 10:12 AM
zack added a reviewer: zack.
zack added a subscriber: zack.
zack added inline comments.
swh/storage/archiver/#director.py#
114 ↗(On Diff #312)

looks like this hunk shouldn't be here, it seem to be including a #backup_file# in the diff

swh/storage/archiver/director.py
87

there seems to be a couple of naming inconsistencies in this function:

  1. the plural, which is not used in other functions that yield multiple contents (e.g., get_unarchived_content_batch)
  2. the lack of trailing _id, whereas the function does return IDs and not actual content

so perhaps it should be renamed to _get_unarchived_content_id (?)

Note: I haven't checked the complete version of the file, so it might be in need of a more general swipe of naming uniformation.

swh/storage/archiver/storage.py
78
  • "starting after" -> "starting at"
  • you should specify if previous_content is included or excluded when restarting from a previous point
  • given we're talking paging, you should specify in which order the contents are returned

More generally, I suggest to use a more consist terminology for pagination: the SQL LIMIT/OFFSET paradigm seems to be a good example to follow here. How about: limit=1000, offset=None (i.e., also dropping the trailing "_content" from the offset argument)?

swh/storage/archiver/worker.py
55 ↗(On Diff #312)

this seems to be a left over

swh/storage/db.py
808–809

same comments about the function prototype as above

This revision now requires changes to proceed.Aug 10 2016, 10:12 AM
qcampos updated this revision to Diff 321.Aug 10 2016, 2:30 PM
qcampos edited edge metadata.
qcampos marked 3 inline comments as done.

Perform some minor corrections.
There is still some work to do about the prototypes.

qcampos marked an inline comment as done.Aug 10 2016, 2:32 PM
qcampos added inline comments.
swh/storage/archiver/storage.py
78

Won't the term offset be misleading about the type of the variable ? As the current previous_content variable is a sha1, and offset may sound like an integer.

zack requested changes to this revision.Aug 12 2016, 10:56 AM
zack edited edge metadata.

Looks good, only a minor terminology change requested, then this could go in.

swh/storage/archiver/storage.py
78

Good point.
Then, as a last change (pun, intended), I propose to rename "previous_content" to "last_content". It's more appropriate, and note how you describe the meaning of the "previous_content" argument invariable as "*last* content retrieved" :-)

This revision now requires changes to proceed.Aug 12 2016, 10:56 AM
qcampos updated this revision to Diff 326.Aug 12 2016, 1:34 PM
qcampos edited edge metadata.
qcampos marked 2 inline comments as done.

Rename previous_content to last_content.

qcampos marked 2 inline comments as done.Aug 12 2016, 1:34 PM
qcampos updated this revision to Diff 329.Aug 12 2016, 1:47 PM
qcampos edited edge metadata.

Merge with origin

qcampos updated this revision to Diff 331.Aug 12 2016, 2:43 PM

Correct the named argument to use last_content.

zack accepted this revision.Aug 16 2016, 10:31 AM
zack edited edge metadata.
This revision is now accepted and ready to land.Aug 16 2016, 10:31 AM
qcampos updated this revision to Diff 334.Aug 16 2016, 4:54 PM
qcampos edited edge metadata.

Update the diff due to rebase on top of origin.

This revision now requires review to proceed.Aug 16 2016, 4:54 PM
olasd accepted this revision.Aug 16 2016, 4:58 PM
olasd edited edge metadata.
This revision is now accepted and ready to land.Aug 16 2016, 4:58 PM
Closed by commit rDSTO48f4c05b66b7: Make the archiver use content_archive.num_present (authored by qcampos). · Explain WhyAug 16 2016, 4:59 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
swh/
storage/
archiver/
29 lines
33 lines
65 lines

Diff 331

View Options

swh/storage/archiver/director.py

Loading...
View Options

swh/storage/archiver/storage.py

Show First 20 Lines • Show All 47 Lines • ▼ Show 20 Lines def content_archive_get(self, content_id, cur=None):
Yields: Yields:
A tuple (content_id, present_copies, ongoing_copies), where A tuple (content_id, present_copies, ongoing_copies), where
ongoing_copies is a dict mapping copy to mtime. ongoing_copies is a dict mapping copy to mtime.
""" """
return self.db.content_archive_get(content_id, cur) return self.db.content_archive_get(content_id, cur)
@db_transaction_generator @db_transaction_generator
def content_archive_get_copies(self, previous_content=None, limit=1000, def content_archive_get_copies(self, last_content=None, limit=1000,
cur=None): cur=None):
"""Get the list of copies for `limit` contents starting after """ Get the list of copies for `limit` contents starting after
`previous_content`. `last_content`.
Args: Args:
previous_content: sha1 of the last content retrieved. May be None last_content: sha1 of the last content retrieved. May be None
to start at the beginning. to start at the beginning.
limit: number of contents to retrieve. Can be None to retrieve all limit: number of contents to retrieve. Can be None to retrieve all
objects (will be slow). objects (will be slow).
Yields: Yields:
A tuple (content_id, present_copies, ongoing_copies), where A tuple (content_id, present_copies, ongoing_copies), where
ongoing_copies is a dict mapping copy to mtime. ongoing_copies is a dict mapping copy to mtime.
""" """
yield from self.db.content_archive_get_copies(previous_content, limit, yield from self.db.content_archive_get_copies(last_content, limit,
cur) cur)
@db_transaction_generator
def content_archive_get_unarchived_copies(
self, retention_policy, last_content=None,
limit=1000, cur=None):
zackUnsubmitted
Done
Inline Actions
  • "starting after" -> "starting at"
  • you should specify if previous_content is included or excluded when restarting from a previous point
  • given we're talking paging, you should specify in which order the contents are returned

More generally, I suggest to use a more consist terminology for pagination: the SQL LIMIT/OFFSET paradigm seems to be a good example to follow here. How about: limit=1000, offset=None (i.e., also dropping the trailing "_content" from the offset argument)?

zack: - "starting after" -> "starting at" - you should specify if previous_content is included or…
qcamposAuthorUnsubmitted
Done
Inline Actions

Won't the term offset be misleading about the type of the variable ? As the current previous_content variable is a sha1, and offset may sound like an integer.

qcampos: Won't the term //offset// be misleading about the type of the variable ? As the current…
zackUnsubmitted
Done
Inline Actions

Good point.
Then, as a last change (pun, intended), I propose to rename "previous_content" to "last_content". It's more appropriate, and note how you describe the meaning of the "previous_content" argument invariable as "*last* content retrieved" :-)

zack: Good point. Then, as a last change (pun, intended), I propose to rename "previous_content" to…
""" Get the list of copies for `limit` contents starting after
`last_content`. Yields only copies with number of present
smaller than `retention policy`.
Args:
last_content: sha1 of the last content retrieved. May be None
to start at the beginning.
retention_policy: number of presentcopies required.
limit: number of contents to retrieve. Can be None to retrieve all
objects (will be slow).
Yields:
A tuple (content_id, present_copies, ongoing_copies), where
ongoing_copies is a dict mapping copy to mtime.
"""
yield from self.db.content_archive_get_unarchived_copies(
retention_policy, last_content, limit, cur)
@db_transaction @db_transaction
def content_archive_update(self, content_id, archive_id, def content_archive_update(self, content_id, archive_id,
new_status=None, cur=None): new_status=None, cur=None):
""" Update the status of an archive content and set its mtime to now """ Update the status of an archive content and set its mtime to now
Change the mtime of an archived content for the given archive and set Change the mtime of an archived content for the given archive and set
it's mtime to the current time. it's mtime to the current time.
Show All 9 Lines
View Options

swh/storage/db.py

Loading...
About · Developer information · Legal