Move all APIs to use the Twisted-managed connection #184
Conversation
jeremycline
added
no-mergify
jeremycline
force-pushed
the
crochet-sync-api
branch
3 times, most recently
from
b5108aa
to
ebcf8a4
Compare
Codecov Report
@@ Coverage Diff @@
## master #184 +/- ##
==========================================
+ Coverage 94.89% 95.42% +0.53%
==========================================
Files 14 13 -1
Lines 1450 1312 -138
Branches 189 158 -31
==========================================
- Hits 1376 1252 -124
+ Misses 55 45 -10
+ Partials 19 15 -4
Continue to review full report at Codecov.
|
jeremycline
force-pushed
the
crochet-sync-api
branch
from
ebcf8a4
to
438d271
Compare
jeremycline
changed the title
jeremycline
force-pushed
the
crochet-sync-api
branch
2 times, most recently
from
8871dae
to
0746d68
Compare
|
@abompard I squashed the one known issue I had with this PR and I think it's ready for an initial review. |
jeremycline
force-pushed
the
crochet-sync-api
branch
from
0746d68
to
513de36
Compare
| # used in the code base which is equivalent to 68 years, which I believe to | ||
| # be an acceptably long amount of time. If you hit this limit you | ||
| # should know that I'll feel no sympathy for you as I'll almost | ||
| # certainly be dead. |
|
|
||
| @pytest_twisted.inlineCallbacks | ||
| def delayed_publish(): | ||
| """Let's the consumer have time to setup.""" |
There was a problem hiding this comment.
Parse error here. Maybe a 's to remove?
| sock.bind(("", 0)) | ||
| config.conf["amqp_url"] = "amqp://localhost:{port}/".format( | ||
| port=sock.getsockname()[1] | ||
| ) |
| client = yield serv.getFactory().whenConnected() | ||
| channel = yield client._allocate_channel() | ||
| assert channel._delivery_confirmation is True | ||
| serv.stopService() |
There was a problem hiding this comment.
Should the startService and stopService be yielded?
There was a problem hiding this comment.
startService doesn't return a deferred, but you're right about stopService. Fixed.
| "exclusive": False, | ||
| "auto_delete": False, | ||
| "arguments": {"x-expires": 60 * 1000}, | ||
| } |
There was a problem hiding this comment.
This definition is re-used plenty of times throughout the file, maybe it could be factored.
| queues = [ | ||
| { | ||
| "queue": queue, | ||
| "auto_delete": True, |
There was a problem hiding this comment.
This was false before, is there a reason to set it to True here?
There was a problem hiding this comment.
It looks like it was always true and I just didn't change it when I merged the two integration test sets, I've fixed it and will also pull out the declaration into a fixture.
|
Okay, I've addressed the PR comments. If you're happy with the general shape of things, I'll resolve the merge conflicts and see about adding a bit more test coverage and we should be ready. I already packaged crochet for Fedora and we can built it in the infra repository for epel7. |
|
Yeah I'm happy with the state of things, and happy to have fewer codepaths :-) Thanks! |
jeremycline
force-pushed
the
crochet-sync-api
branch
2 times, most recently
from
1e38a9d
to
ca4b0f4
Compare
This introduces crochet[0], a small library designed to make it easy to provide synchronous APIs using asynchronous Twisted APIs. This modifies the `fedora_messaging.api.consume` and `fedora_messaging.api.publish` calls to use the Twisted protocol. There are several advantages here: 1. The synchronous and asynchronous APIs use the same code which means there's fewer opportunities for them to differ in behavior and less code to maintain. 2. Both APIs didn't really work well because they could not heartbeat in the background. This lets them do so because the connection is handled in Twisted in a separate crochet-managed thread. While the performance improvements are likely modest, it won't lead to so many TCP connections being killed by firewalls or the broker. The downside is it introduces the complexity of threads to the API which makes it less obvious what is going on. There are a few minor changes here which slightly change the APIs: 1. Publishing now raises a PublishTimeout when the timeout is reached (30 seconds by default). 2. Previously, the Twisted consume API did not validate arguments like the synchronous version did, so it now raises a ValueError on invalid arguments instead of crashing in some undefined way. 3. Calling publish from the Twisted reactor thread now raises an exception instead of blocking the reactor thread. Although all these are small changes, I think this should be in a 2.0 release. In practice I don't think any of these issues will bite anyone so upgrading shouldn't be a problem for anyone. This does not move the CLI back to the synchronous API (yet). Signed-off-by: Jeremy Cline <jcline@redhat.com>
Signed-off-by: Jeremy Cline <jcline@redhat.com>
Signed-off-by: Jeremy Cline <jcline@redhat.com>
Signed-off-by: Jeremy Cline <jcline@redhat.com>
jeremycline
force-pushed
the
crochet-sync-api
branch
from
ca4b0f4
to
ebae73e
Compare
jeremycline
removed
the
no-mergify
|
Okay, I've rebased and added a couple more tests. The lack of coverage is in a compatibility block for EL7 which is difficult to test in CI, but would be covered by tests run in EL7. I'm inclined to leave them as-is until we drop support for that. The other spot I couldn't easily get coverage was in the publish code that handled connection errors during the publish. I could mock it all out and test that, but I'd prefer a test against an actual broker, I've just not come up with a neat way to do it yet. Anyway, I think this is ready for a final look over and merge. I'll see about getting the publish CLI branch I have rebased and pushed since that will include a few backwards incompatible code changes to dump/load format of messages (which I think no one is using). I'll also see about dropping the old twisted protocol code since it's a major release and I'm also pretty sure no one is using it. |
This introduces crochet[0], a small library designed to make it easy to
provide synchronous APIs using asynchronous Twisted APIs. This modifies
the
fedora_messaging.api.consumeandfedora_messaging.api.publishcalls to use the Twisted protocol.
There are several advantages here:
The synchronous and asynchronous APIs use the same code which means
there's fewer opportunities for them to differ in behavior and less
code to maintain.
Both APIs didn't really work well because they could not heartbeat in
the background. This lets them do so because the connection is
handled in Twisted in a separate crochet-managed thread. While the
performance improvements are likely modest, it won't lead to so many
TCP connections being killed by firewalls or the broker.
The downside is it introduces the complexity of threads to the API which
makes it less obvious what is going on.
There are a few minor changes here which slightly change the APIs:
Publishing now raises a PublishTimeout when the timeout is reached
(30 seconds by default).
Previously, the Twisted consume API did not validate arguments like
the synchronous version did, so it now raises a ValueError on invalid
arguments instead of crashing in some undefined way.
Calling publish from the Twisted reactor thread now raises an
exception instead of blocking the reactor thread.
Although all these are small changes, I think this should be in a 2.0
release. In practice I don't think any of these issues will bite anyone
so upgrading shouldn't be a problem for anyone.
This does not move the CLI back to the synchronous API (yet).
This fixes #171 by unifying the async and sync APIs.