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 Graph service
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

zack created this revision.Sep 8 2019, 11:13 AM
zack added a reviewer: seirl.Sep 8 2019, 11:13 AM
zack updated this revision to Diff 6644.Sep 8 2019, 11:15 AM
  • pid.py: avoid importing unusued mmap constants
zack updated this revision to Diff 6645.Sep 8 2019, 11:24 AM
  • cli.py: avoid importing unused PID_BIN_SIZE constant
douardda requested changes to this revision.Sep 9 2019, 10:07 AM
douardda added a subscriber: douardda.

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

swh/graph/cli.py
95–97

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
102–108

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–97

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.

seirl added inline comments.Sep 9 2019, 11:31 AM
swh/graph/pid.py
102–108

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

vlorentz added inline comments.Sep 9 2019, 11:39 AM
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 updated this revision to Diff 6646.Sep 9 2019, 11:43 AM
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

zack updated this revision to Diff 6647.Sep 9 2019, 11:47 AM
  • 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
102–108

I love these two together! Fixed

seirl accepted this revision.Sep 11 2019, 5:17 PM
zack removed a reviewer: douardda.Sep 12 2019, 9:52 AM
zack marked an inline comment as done.
This revision is now accepted and ready to land.Sep 12 2019, 9:52 AM
zack updated this revision to Diff 6683.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
zack updated this revision to Diff 6684.Sep 12 2019, 10:01 AM
  • 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.