Relates to: T3530
Details
Details
- Reviewers
vlorentz - Group Reviewers
Reviewers - Commits
- rDOBJS5904f7c155de: winery: implement read/write throttling
Diff Detail
Diff Detail
- Repository
- rDOBJS Object storage
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Comment Actions
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: T3530See https://jenkins.softwareheritage.org/job/DOBJS/job/tests-on-diff/105/ for more details.
Comment Actions
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
Comment Actions
Looks good, as usually.
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
- Please add docstrings to classes in throttler.py, it's really not obvious how they work.
- 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 | ||
|---|---|---|
| 94 | ditto | |
| 101 | ||
| 102 | 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 | ||
| 247 | that's because it removes 0 on each tick, right? Could you mention this in a comment here? | |
| 263 | (same for the two instances below) | |
| swh/objstorage/backends/winery/database.py | ||
|---|---|---|
| 94 | I must be missing the first instance this "ditto" refers to. | |
| 101 | Done. | |
| 102 | Done. | |
| swh/objstorage/tests/test_objstorage_winery.py | ||
| 247 | All comments in this test were rewritten for clarity | |
| 263 | Changed to called_once because it's not possible to assert the value of the argument. | |
Comment Actions
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.
Comment Actions
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: T3530See https://jenkins.softwareheritage.org/job/DOBJS/job/tests-on-diff/106/ for more details.
Comment Actions
- Remove spurious table added to sharedbase
- Reduce the number of PG in the Ceph cluster: 100 is enough for test purposes
Comment Actions
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: T3530See https://jenkins.softwareheritage.org/job/DOBJS/job/tests-on-diff/107/ for more details.