PR#957: New multicall interface - koji

koji

#957 New multicall interface

Merged 4 years ago by mikem. Opened 5 years ago by mikem.

mikem/koji multicall-tracker-2 into master

No commits found

mikem commented 5 years ago

The original multicall interface is less than optimal:

It is modal. Once session.multicall is set, the session cannot be used normally
Confusing to have both session.multicall and session.multiCall()
Processing the return value is tedious

These changes create a new implementation outside of ClientSession. The old way will still work.

With this new implementation:

a multicall is tracked as an instance of MultiCallSession
the original session is unaffected
multiple multicalls can be managed in parallel, if desired
MultiCallSession behaves more or less like a session in multicall mode
method calls return a VirtualCall instance that can later be used to access the result
MultiCallSession can be used as a context manager, ensuring that the calls are executed

Here is one example of possible use:

with session.multicall(strict=True) as m:
    for package in to_add:
        m.packageListAdd(tag, package, options.owner, **opts)

Here is an example that make use of the VirtualCall values:

tinfos = {}
with session.multicall(strict=True) as m:
    for task_id in task_ids:
        tinfos[task_id] = m.getTaskInfo(task_id)
for task_id, tinfo in tinfos.items():
    tinfo = tinfo.result

mikem commented 5 years ago

About the multicall attribute....

We already had both multicall and multiCall. I was hesitant to add yet another similar sounding attribute to the ClientSession class to access this functionality. Luckily, it was possible to redefine the multicall attribute to serve both as a settable boolean (original behavior) and a callable (new behavior).

mikem commented 5 years ago

This seems to work well, but I'd appreciate any feedback.

I still need to add some docs and additional unit tests for this.

mikem commented 5 years ago

Also, I should add a batch option for parity with #900

julian8628 commented on line 54 of koji/__init__.py 5 years ago

should MultiCallNotReady inherit GenericError?

tkopecek commented 5 years ago

Nice, I like the context manager approach.

Does it make sense to store arguments in VirtualCall as original args/kwargs for some possible introspection? I know, that it is really simple to recompose them from params, but it could be more human-friendly interface (e.g defining str for it printing original call). Also, isn't the right place for encode_args inside format as it is the only place where it matters?

mikem commented 5 years ago

should MultiCallNotReady inherit GenericError?

Not really, because this is client-only code. GenericError and its children are for exceptions that the hub will return as faults.

Granted, I'm not super happy with Koji's exception hierarchy...

mikem commented 5 years ago

Does it make sense to store arguments in VirtualCall as original args/kwargs for some possible introspection?

Yeah, I was pondering that. I'll adjust that when I get a chance

2 new commits added

cleanup
store args/kwargs in VirtualCall instances

5 years ago

1 new commit added

stub unit test

5 years ago

1 new commit added

provide multicall attribute for backwards compat

5 years ago

rebased onto a0d5971791552ec2bf1ce5c07d5f20d49fd3dccd

4 years ago

mikem commented 4 years ago

Rebased with updates:

expanded unit test a bit
added batch option for parity with #900
used the new interface to add multicall to a few more cli commands

mikem commented 4 years ago

Still need to document the new interface, but I'd like to merge #1358 first since that also contains some multicall docs (that will need updating after this).

rebased onto 54da20e

4 years ago

mikem commented 4 years ago

rebased with docs updates

mikem commented 4 years ago

@tkopecek @julian8628 I think this is about ready. Appreciate any feedback.

tkopecek commented on line 19 of docs/source/writing_koji_code.rst 4 years ago

"rather than"?

tkopecek commented 4 years ago

:thumbsup:

Question, maybe for 1.19 - would we like to replace old implementation with special instance of MultiCallSession? There is a lot of duplicated code and if we ensure, that all calls when session.multicall is true are caught by this special instance, it should be less code/maintenance.

1 new commit added