Page MenuHomeSoftware Heritage

Add Storage.content_get_partition endpoint, to replace content_get_range.
ClosedPublic

Authored by vlorentz on Nov 22 2019, 4:10 PM.

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

vlorentz created this revision.Nov 22 2019, 4:10 PM
ardumont requested changes to this revision.Dec 16 2019, 9:10 AM
ardumont added a subscriber: ardumont.

Looks good ;)

I have mostly concerns about missing types and consistency naming.

swh/storage/api/client.py
58 ↗(On Diff #8064)

'count' instead of limit, you asked me yourself [1]

[1] D2457#57977

swh/storage/in_memory.py
312

type?

314

must be in [0, nb_partitions[
or
must be in [0, nb_partitions - 1]

?

316

or the result order.

329

retrieving

swh/storage/tests/storage_data.py
16

Why do we need this?

(maybe add a comment about it)

swh/storage/utils.py
35

Why if instead of assert as you use sometimes?

46
start = bounds[0].to_bytes(nb_bytes, 'big')
end = None if i == n-1 else bounds[1].to_bytes(nb_bytes, 'big')
return (start, end)

or

start = bounds[0].to_bytes(nb_bytes, 'big')
if i == n-1:
    end = None
else:
    end = bounds[1].to_bytes(nb_bytes, 'big')
return (start, end)
This revision now requires changes to proceed.Dec 16 2019, 9:10 AM
vlorentz added inline comments.Dec 16 2019, 1:59 PM
swh/storage/utils.py
35

assert is for things that should never happen, if... raise is for errors that may happen. But yeah sometimes I use assert too much

vlorentz added inline comments.Dec 17 2019, 1:53 PM
swh/storage/tests/storage_data.py
16

It's how __getattr__ works: "This method should either return the (computed) attribute value or raise an AttributeError exception." https://docs.python.org/3/reference/datamodel.html#object.__getattr__

Else, getattr() and hasattr() on the object fail, eg.

/usr/lib/python3.7/doctest.py:932: in find
    self._find(tests, obj, name, module, source_lines, globs, {})
.tox/py3/lib/python3.7/site-packages/_pytest/doctest.py:445: in _find
    self, tests, obj, name, module, source_lines, globs, seen
/usr/lib/python3.7/doctest.py:991: in _find
    if ((inspect.isroutine(inspect.unwrap(val))
.tox/py3/lib/python3.7/site-packages/_pytest/doctest.py:408: in _mock_aware_unwrap
    return real_unwrap(obj, stop=_is_mocked)
/usr/lib/python3.7/inspect.py:511: in unwrap
    while _is_wrapper(func):
/usr/lib/python3.7/inspect.py:505: in _is_wrapper
    return hasattr(f, '__wrapped__') and not stop(f)
.tox/py3/lib/python3.7/site-packages/swh/storage/tests/storage_data.py:14: in __getattr__
    v = globals()[key]
E   KeyError: '__wrapped__'
vlorentz updated this revision to Diff 8753.Dec 17 2019, 1:59 PM

apply comments

vlorentz marked 5 inline comments as done.Dec 17 2019, 1:59 PM
ardumont accepted this revision.Dec 17 2019, 2:22 PM
This revision is now accepted and ready to land.Dec 17 2019, 2:22 PM