Page MenuHomeSoftware Heritage

pid maps: add limited support for updatable maps
ClosedPublic

Authored by zack on Sun, Sep 8, 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.Sun, Sep 8, 11:13 AM
zack added a reviewer: seirl.Sun, Sep 8, 11:13 AM
zack updated this revision to Diff 6644.Sun, Sep 8, 11:15 AM
  • pid.py: avoid importing unusued mmap constants
zack updated this revision to Diff 6645.Sun, Sep 8, 11:24 AM
  • cli.py: avoid importing unused PID_BIN_SIZE constant
douardda requested changes to this revision.Mon, Sep 9, 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.Mon, Sep 9, 10:07 AM
vlorentz added inline comments.
swh/graph/tests/test_pid.py
54

Why this change?

zack marked 2 inline comments as done.Mon, Sep 9, 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.Mon, Sep 9, 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.Mon, Sep 9, 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.Mon, Sep 9, 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.Mon, Sep 9, 11:47 AM
  • pid.py: use a dict for more idiomatic file mode check
zack marked 2 inline comments as done.Mon, Sep 9, 11:47 AM
zack added inline comments.
swh/graph/pid.py
102–108

I love these two together! Fixed

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