Page MenuHomeSoftware Heritage

winery: implement read/write throttling
ClosedPublic

Authored by dachary on Jan 22 2022, 4:14 PM.

Details

Summary

Relates to: T3530

Diff Detail

Repository
rDOBJS Object storage
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26287
Build 41097: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 41096: arc lint + arc unit

Event Timeline

Build is green

Patch application report for D7022 (id=25449)

Rebasing onto 9a88cafbc7...

Current branch diff-target is up to date.
Changes applied before test
commit c44808c6df2ae8f6e3fa0a4f45663d52182d37d2
Author: Loïc Dachary <loic@dachary.org>
Date:   Fri Jan 21 19:12:59 2022 +0100

    winery: implement read/write throttling
    
    Relates to: T3530

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

This patch contains:

  • A trivial refactor of the Database class to keep it DRY while using it in a third context
  • The implementation of the IO throttling logic and basic benchmark code, with an update to the documentation to provide a high level view and help the reviewer before diving into the details

I would be grateful for a quick review before actually running the benchmark, in case something horribly wrong strikes someone

Looks good, as usually.

Some generic readability requests, though:

  1. Could you add type signatures to all the new functions? mypy is going to (rightfully) complain about create_tables, because subclasses use a different signature; you should rename the definition in Database
  2. Please add docstrings to classes in throttler.py, it's really not obvious how they work.
  3. In logging statements (especially debug level), delegate substitution to the logger instead of using f-string. eg. replace logger.debug(f"{self.rowid}: upload {bytes}/s") with logger.debug("%d: upload %s/s", self.rowid, self.bytes)
swh/objstorage/backends/winery/database.py
95

ditto

102
103

just to make sure it isn't accidentally called despite @abc.abstractmethod (eg. if a child class uses super().lock())

swh/objstorage/backends/winery/roshard.py
1
swh/objstorage/tests/test_objstorage_winery.py
248

that's because it removes 0 on each tick, right? Could you mention this in a comment here?

264

(same for the two instances below)

dachary added inline comments.
swh/objstorage/backends/winery/database.py
95

I must be missing the first instance this "ditto" refers to.

102

Done.

103

Done.

swh/objstorage/tests/test_objstorage_winery.py
248

All comments in this test were rewritten for clarity

264

Changed to called_once because it's not possible to assert the value of the argument.

dachary marked 5 inline comments as done.

Looks good, as usually.

Thanks for the kind words :-) And even more for the quick review, as always. I hopefully adressed your inline comments with this new version of the commit.

Some generic readability requests, though:

Could you add type signatures to all the new functions? mypy is going to (rightfully) complain about create_tables, because subclasses use a different signature; you should rename the definition in Database

It was ill advised on my part to overload this function in this way. It now has a consistent signature instead.

Please add docstrings to classes in throttler.py, it's really not obvious how they work.

Done.

In logging statements (especially debug level), delegate substitution to the logger instead of using f-string. eg. replace logger.debug(f"{self.rowid}: upload {bytes}/s") with logger.debug("%d: upload %s/s", self.rowid, self.bytes)

Done.

Build is green

Patch application report for D7022 (id=25453)

Rebasing onto 9a88cafbc7...

Current branch diff-target is up to date.
Changes applied before test
commit 22c58851ada9f187aa202e07a077fcacc9fa912c
Author: Loïc Dachary <loic@dachary.org>
Date:   Fri Jan 21 19:12:59 2022 +0100

    winery: implement read/write throttling
    
    Relates to: T3530

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

  • Remove spurious table added to sharedbase
  • Reduce the number of PG in the Ceph cluster: 100 is enough for test purposes

Build is green

Patch application report for D7022 (id=25454)

Rebasing onto 9a88cafbc7...

Current branch diff-target is up to date.
Changes applied before test
commit 5904f7c155de8c67d1360a25518fd31a993e72a5
Author: Loïc Dachary <loic@dachary.org>
Date:   Fri Jan 21 19:12:59 2022 +0100

    winery: implement read/write throttling
    
    Relates to: T3530

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

swh/objstorage/backends/winery/database.py
95

oops, that was about docstrings

swh/objstorage/tests/test_objstorage_winery.py
248

oops, I meant "removes 100"

This revision is now accepted and ready to land.Jan 24 2022, 1:58 PM
dachary added inline comments.
swh/objstorage/backends/winery/database.py
95

:-D

swh/objstorage/tests/test_objstorage_winery.py
248

:-D

This revision was automatically updated to reflect the committed changes.
dachary marked 2 inline comments as done.