Code review comment for lp:~jelmer/bzr/hpss-get-inventories

Revision history for this message
Andrew Bennetts (spiv) wrote :

Jelmer Vernooij wrote:
> Add a HPSS call for ``Repository.iter_inventories``.

I am a bit concerned that by adding verbs like this, with their own ad hoc
record stream-like wire format, that we're growing the maintenance burden
unnecessarily, and not reusing improvements everywhere we could.

I'm glad this is using inventory-deltas. But if it's worth zlib compressing
them here, shouldn't we do that for inventory-deltas from get_record_stream too?

Also, could you implement this via the get_record_stream RPC with a new
parameter that means “just inventories (rather than rev+inv+texts+chks+sigs for
given keys)”?

On one hand it might feel a bit ugly to make one RPC do so many things,
get_record_stream, do so many things, but on the other I think it provides a
useful pressure to keep the interface minimal and consistent (e.g. whether
records are zlib-compressed).

Adding this iter_inventories RPC might be the right thing (although putting
“iter” in the name of an RPC feels weird to me, that's surely a property of the
Python API rather than a property of the RPC), but I'd like hear what you think
about the tradeoffs of doing that vs. extending get_record_stream.

-Andrew.

« Back to merge proposal