koji

#957 New multicall interface
Merged by mikem. Opened by mikem.
mikem/koji multicall-tracker-2  into  master

Download 957.patch

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

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

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

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

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

should MultiCallNotReady inherit GenericError?

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?

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

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

1 new commit added

  • stub unit test

1 new commit added

  • provide multicall attribute for backwards compat

rebased onto a0d5971791552ec2bf1ce5c07d5f20d49fd3dccd

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

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 54da20e253ce379371bb23ce92f6f905e5c07fae

rebased with docs updates

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

"rather than"?

: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

  • fix typo

Question, maybe for 1.19 - would we like to replace old implementation with special instance of MultiCallSession?

Sure. I was initially hesitant to touch the original code, but once this has seen a real release it should be fine.

Also it is an important change, so there should be an issue linked before merging (Just to see it in roadmap, etc.)

Commit d612eac1 fixes this pull-request

Pull-Request has been merged by mikem

there should be an issue linked

Ah good point. I'll add one.

Fixes: #1501
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.