Page MenuHomeSoftware Heritage

pid maps: add limited support for updatable maps
ClosedPublic

Authored by zack on Sep 8 2019, 11:13 AM.

Diff Detail

Repository
rDGRPH Compressed graph representation
Branch
feature/writable-maps
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7727
Build 11104: tox-on-jenkinsJenkins
Build 11103: arc lint + arc unit

Event Timeline

  • pid.py: avoid importing unusued mmap constants
  • cli.py: avoid importing unused PID_BIN_SIZE constant
douardda added a subscriber: douardda.

Looks globally ok to me, so feel free to ignore my comments.

swh/graph/cli.py
95–100

This is not part of the diff, but I tend to find weird to use required options in a cli tool...
This should be an argument IMHO. Or at least let the option have a default value.

swh/graph/pid.py
101–107

I tend to prefer a dict-based approach for this kind of stuff like (not properly formatted):

MODES = {'rb': os.O_RDONLY, 'wb': os.O_RDWR | os.O_CREAT, 'rb+': os.O_RDWR}
os_mode = MODES.get(mode)
This revision now requires changes to proceed.Sep 9 2019, 10:07 AM
vlorentz added inline comments.
swh/graph/tests/test_pid.py
54

Why this change?

zack marked 2 inline comments as done.Sep 9 2019, 10:31 AM
zack added inline comments.
swh/graph/cli.py
95–100

That's a fair point. But when you start having multiple positional argument, it's a pain to remember which one goes where if there isn't an obvious order (e.g., cp's src/dst). So I'd rather keep it like this for now.

swh/graph/tests/test_pid.py
54

Readability. The order here doesn't matter, as there is a sorted in the function implementation anyway. But it was a pain to read as a default argument in a non-alphabetic order.

swh/graph/pid.py
101–107

(This also allows you to check if mode not in MODES at the beginning.

swh/graph/tests/test_pid.py
54

It's still not alphabetic. (ori is after rev).

And the original order made sense because it's a topological order.

54

Actually no, in the original order, rev and rel were in the wrong order wrt the usual order

zack marked 4 inline comments as done.
  • test_pid.py: fix alphabetic ordering of node types
swh/graph/tests/test_pid.py
54

fixed now

  • pid.py: use a dict for more idiomatic file mode check
zack marked 2 inline comments as done.Sep 9 2019, 11:47 AM
zack added inline comments.
swh/graph/pid.py
101–107

I love these two together! Fixed

zack marked an inline comment as done.
This revision is now accepted and ready to land.Sep 12 2019, 9:52 AM
  • pid maps: add limited support for updatable maps
  • CLI: make restore of int->pid maps use mmap writing instead of seek
  • pid.py: avoid importing unusued mmap constants
  • cli.py: avoid importing unused PID_BIN_SIZE constant
  • test_pid.py: fix alphabetic ordering of node types
  • pid.py: use a dict for more idiomatic file mode check
  • pid.py: avoid importing unused mmap constants
  • cli.py: avoid importing unused PID_BIN_SIZE constant
  • test_pid.py: fix alphabetic ordering of node types
  • pid.py: use a dict for more idiomatic file mode check
This revision was automatically updated to reflect the committed changes.