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

Hi Andrew,

> 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?
It's a pretty big inventory delta in this case - for something like gcc it matters for the delta between null: and the initial revision that was requested. I haven't investigated whether it would help for record streams too, but I imagine it would have less of an effect.

> 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.

With regard to the name: Most of the other calls seem to be named after the equivalent method in the client / server, that's why I went with Repository.iter_inventories. I'm not particularly tied to that name, something like "Repository.get_inventories" or "Repository.stream_inventories" seems reasonable to me too.

I'm not sure whether this should be part of get_record_stream. I have a hard time understanding the get_record_stream code as it is, so I'd rather not make it even more complex by adding more parameters - for example, get_record_streams seem to be dependent on the repository on-disk format to an extent - the inventory stream is not, as it's using inventory deltas. In other words, adding another verb was simpler, while keeping it all understandable for a mere mortal like me. :-)

Either way, I agree we should be reusing more code between the work I've done recently and the existing record stream
 calls. But it seems to me that would best be done by refactoring so they e.g. use the same code for sending a stream of blobs of indeterminate length, rather than by all being a part of the same verb.

What do you think?

Cheers,

Jelmer