[PATCH 3 of 3] listkeypattern: add listkeypattern wireproto method

Augie Fackler raf at durin42.com
Wed Aug 17 09:12:54 EDT 2016


On Tue, Aug 16, 2016 at 06:39:38PM -0700, Gregory Szorc wrote:
> On Tue, Aug 16, 2016 at 5:22 PM, Pierre-Yves David <pierre-yves.david at ens-lyon.org> wrote:
> > On 08/14/2016 07:17 PM, Gregory Szorc wrote:
[snip]
> >> I think introducing a new wire protocol command is the correct way to
> >> solve this problem (as opposed to introducing a new argument on the
> >> existing command).
> >>
> >> However, if we're introducing a new wire protocol command for obtaining
> >> pushkey values, I think we should improve deficiencies in the response
> >> encoding rather than propagate its problems.
> >>
> >> The "listkeys" response encoding can't transmit the full range of binary
> >> values. This can lead to larger (and slower) responses sizes. For
> >> example, a number of pushkey namespaces exchange lists of nodes. These
> >> have to be represented as hex instead of binary. For pushkey namespaces
> >> like phases or obsolescence that can exchange hundreds or thousands of
> >> nodes, the overhead can add up.
> >>
> >> I think the response from a new listkeys command should be using framing
> >> to encode the key names and values so the full range of binary values
> >> can be efficiently transferred. We may also want a special mechanism to
> >> represent a list of nodes, as avoiding the overhead of framing on fixed
> >> width values would be desirable.
> >>
> >> Of course, at the point you introduce a new response encoding, we may
> >> want to call the command "listkeys2." If others agree, I can code up an
> >> implementation and you can add the patterns functionality on top of it.
> >>
> >
> > Sorry to be a bit late to the discussion.
> >
> > I don't think we should introduce a new wire-protocol command for this.
> >
> > Individual listkey call have been a large source of race condition and
> > related issue. Bundle2 is able to carry listkey.pushkey call just fine and
> > I think we should prioritize its usage. As bundle2 already have framing, we
> > could just use your better encoding with bundle2 directly.
> > However, we should probably push things further and use dedicated part for
> > commonly used request. Having dedicated type will help more semantic reply
> > and request. The series we are commenting on is a good example of that need
> > for "pattern" here pretty much only apply to bookmark, having a dedicated
> > "channel" (within bundle2) for this would make is painless to add any new
> > arguments we could need.
> >
> > TL;DR; I don't think we should touch listkey.pushkey. add a bundle2 part
> > dedicated to bookmark instead.
> >
>
> I have serious objections to stuffing yet more functionality into the
> "getbundle" wire protocol command. That command is quickly becoming a "god
> object" [1]. I'd rather we freeze that wire protocol command and offer
> equivalent functionality under new, better designed commands.

In general, I agree with your feeling on the bundle kind of being a
god object, but for bookmarks in particular Pierre-Yves is absolutely
correct. I think pushkey makes a ton of sense for things like marking
code review state, which can sensibly be done outside a transaction
(perhaps only sensibly done outside a repo history transaction? I'm
not sure), but in general most of the things we historically used
pushkey for should be in the same transaction as the rest of a push.

As a bonus, a dedicated bundle2 part for bookmarks would let us
properly resolve https://bz.mercurial-scm.org/show_bug.cgi?id=5165, so
we should probably just do that. I hadn't thought of that as a
solution, but it seems like the clear winner for that case now that
someone's said it.

What are use cases for pushkey beyond things that should be part of a
repo transaction? Can we come up with enough of those that would like
binary data payloads to justify the new command?

>
> In general, I'm opposed to adding arguments to wire protocol commands.
> Introducing an argument requires advertising a server capability otherwise
> clients may send an unknown argument to an incompatible server, which the
> server will reject. At the point you introduce a new argument/capability,
> you've effectively introduced a new variation of the wire protocol command.
> You might as well call it something else so the semantics of each wire
> protocol are well-defined and constant over time. If nothing else, this
> makes the client/server code easier to understand. I only need to point out
> wireproto.getbundle() and wireproto.unbundle() for examples of overly
> complicated code as a result of introducing wire protocol command arguments
> (notably support for bundle2).
>
> I'm not strongly opposed to the idea of making bundle2 a generic framing
> protocol. I think we could do better. (I raised a number of concerns with
> bundle2 during its implementation phase. In fact, one of them was that
> bundle2 as a generic format for data in transport and at rest is not ideal:
> there are various aspects you want to optimize for and one format cannot
> exceed at all of them. Unfortunately, not much was done because bundle2 was
> already kinda/sorta deployed in a few environments at that time. My fault
> for taking too long to audit the protocol.) I think bundle2 is an OK'ish
> format for data at rest. For a transport protocol, not so much.
>
> If we do use bundle2 as a container for the wire protocol, that doesn't
> mean we can't have multiple wire protocol commands for exchanging bundle2
> payloads.
>
> [1] https://en.wikipedia.org/wiki/God_object

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


More information about the Mercurial-devel mailing list