This is an archive of the discontinued Mercurial Phabricator instance.

repository: define new interface for running commands
ClosedPublic

Authored by indygreg on Apr 11 2018, 7:42 PM.

Details

Summary

Today, the peer interface exposes methods for each command that can
be executed. In addition, there is an iterbatch() API that allows
commands to be issued in batches and provides an iterator over the
results. This is a glorified wrapper around the "batch" wire command.

Wire protocol version 2 supports nicer things (such as batching
any command and out-of-order replies). It will require a more
flexible API for executing commands.

This commit introduces a new peer interface for making command
requests. In the new world, you can't simply call a method on the
peer to execute a command: you need to obtain an object to be used
for executing commands. That object can be used to issue a single
command or it can batch multiple requests. In the case of full duplex
peers, the command may even be sent out over the wire immediately.

There are no per-command methods. Instead, there is a generic
method to call a command. The implementation can then perform domain
specific processing for specific commands. This includes passing
data via a specially named argument.

Arguments are also passed as a dictionary instead of using kwargs.
While
kwargs is nicer to use, we've historically gotten into
trouble using it because there will inevitably be a conflict between
the name of an argument to a wire protocol command and an argument
we want to pass into a function.

Instead of a command returning a value, it returns a future which
will resolve to a value. This opens the door for out-of-order
response handling and concurrent response handling in the version
2 protocol.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

indygreg created this revision.Apr 11 2018, 7:42 PM

After coding up a handful of patches to switch to the new interface, I concede that I like the use of methods on the existing API. However, it is important for protocol version 2 that the peer interface switch to futures. I'm very reluctant to change the return value of peer APIs because that would likely be a difficult API break. I would be OK with exposing attributes on the command executor instance that magically route to commands. That's how the existing iterbatch() interface works. While the code is a bit wonky, it would work.

indygreg planned changes to this revision.Apr 13 2018, 1:23 PM

I'll be making small revisions to the interface docs and implementation.

indygreg updated this revision to Diff 8120.Apr 13 2018, 3:14 PM

We now send non-batchable commands immediately and wrap queued futures so result() will trigger submission. This improves the end-user experience from:

with peer.commandexecutor() as e:
    f = e.callcommand(...)

res = f.result()

to

with peer.commandexecutor() as e:
    res = e.callcommand(...).result()

And in order to preserve streaming on batch responses:

with peer.commandexecutor() as e:
    fs = []
    for x in ...:
        fs.append(...)

    e.sendcommands()
    for f in fs:
        result = f.result()

to

with peer.commandexecutor() as e:
    fs = []
    for x in ...:
        fs.append(e.callcommand(...))

    for f in fs:
        result = f.result()

This later improvement is because the first result() call on any returned future will trigger submission of all queued commands. This also means we can iterate or access the futures in any order. Of course, wire protocol version 1 will resolve them in the order they were issued. But this isn't necessarily true about wire protocol version 2 :)

durin42 accepted this revision.Apr 13 2018, 5:34 PM
This revision is now accepted and ready to land.Apr 13 2018, 5:34 PM
This revision was automatically updated to reflect the committed changes.