Refactor AMQP publishing, expose delivery options #456

mattbennett · 2017-07-20T16:39:18Z

This consolidates AMQP message publishing code into a shared module, and exposes delivery options where appropriate for the built-in DependencyProviders that publish messages (RpcProxy, EventDispatcher, Publisher).

The DependencyProviders now allow certain message and delivery options to be specified at instantiation or publish time, for example:

class Serv:
     
    # publish transient messages, don't confirm receipt, never attempt to retry
    publish = Publisher(delivery_mode=1, use_confirms=False, retry=False)

    @entrypoint
    def meth(self):

        # publish a message with the default options
        self.publish("msg", routing_key="foo")

        # do retry, but only once
        self.publish("msg", routing_key="foo", retry=True, retry_policy={'max_attempts': 1})

It's possible to provide headers and declarations in the same way:

class Serv:

    publish = Publisher(
        # always add this header to published messages
        headers={'foo': 'foo'},
        # ensure these queues and exchanges exist before publishing
        declare=[some_queue, some_exchange]
    )

    @entrypoint
    def meth(self):
        self.publish(
            "msg",
            headers:{'foo': 'FOO', 'bar': 'BAR'},  # extend or override default headers
            declare: [another_queue]  # more entities to declare before publishing
        )

The TestConfigurability tests for the DependencyProviders show which options can be specified for each (e.g. you can't specify a different routing key or exchange for an EventDispatcher).

This feature needs documenting but there's not currently anywhere sensible to describe it. I would like (soon) to add a dedicated page to the docs for each of the AMQP extensions, but this PR is big enough as it is.

simplify event dispatcher by having it reuse the publisher's implementation

time as well as event dispatchers, and test that precedence is correct; adds two new pytest fixtures: amqp_uri and get_message_from_queue (which still need tests)

headers that we want to propagate (e.g. call stack)

…ry-params

to another object that isn't an exchange

davidszotten

nice work!

massive pr so hard to do a thorough review. seems like a sensible refactor and we have pretty good test coverage in general

hard to get a sense of impact on backwards compat

added a few style comments/questions

davidszotten · 2017-07-30T10:49:17Z

nameko/amqp/__init__.py

@@ -0,0 +1,4 @@
+# backwards compat imports
+from .utils import verify_amqp_uri  # noqa: F401


how come we need these lint exemptions?

They're "unused imports" otherwise. I'm only pulling them into this package for a backwards compatibility import of from nameko.amqp import verify_amqp_uri

davidszotten · 2017-07-30T10:50:38Z

nameko/amqp/publish.py

+    """
+
+    delivery_mode = PERSISTENT
+    """


nice way to document this stuff

davidszotten · 2017-07-30T10:56:24Z

nameko/amqp/publish.py

+        self.amqp_uri = amqp_uri
+
+        # publish confirms
+        if use_confirms is not None:


how come you aren't just making the defaults the default parameters here? documentation? subclassing to override defaults? (are we adding too many ways to change settings?)

Several of them have truthy defaults, so it's hard to compare whether a new value has been provided. This is a little more verbose but consistent for every param. I also like that the default values sit next to the docs for each option.

A slightly less verbose option could be the following:

def __init__(self, amqp_uri, **kwargs): self.amqp_uri = amqp_uri self.use_confirms = kwargs.pop('use_confirms', self.use_confirms) ...

Slight disadvantage here is that the sphinx docs would only show **kwargs.

Re: adding too many ways to change settings. Maybe. This does allow subclassing to override defaults as well as at instantiation (and use-time). Subclassing used to be the only way to change these things previously though, and we encourage subclassing for other reasons.

davidszotten · 2017-07-30T10:58:50Z

nameko/messaging.py

 from functools import partial
 from logging import getLogger
+import warnings


should we consider adding a linter to enforce import orders?

Perhaps, although it's going to be another annoying step that frequently gets forgotten and fails the build. Out of order imports are annoying though, and so are casual rearrangements inside other commits.

Could we automatically re-write them on a commit hook? Then the order will always be consistent and any re-arrangement would be made automatically and in its own commit.

i would be -1 on auto-rewriting. status quo is probably fine, though a checking is a fairly fast lint run with e.g isort. also has the bonus of optionally autosorting. something for another day

davidszotten · 2017-07-30T11:00:11Z

nameko/messaging.py

+                "The signature of `Publisher` has changed. The `queue` kwarg "
+                "is now deprecated. You can use the `declare` kwarg "
+                "to provide a list of Kombu queues to be declared. "
+                "See CHANGES, version 2.5.2 for more details. This warning "


wrong version?

Yep, thanks. Shows how long this PR has been in the works!

davidszotten · 2017-07-30T11:01:45Z

nameko/messaging.py

-                    retry_policy=retry_policy, mandatory=mandatory,
-                    **kwargs
-                )
+        propagate_headers = self.get_message_headers(worker_ctx)


how come this var is called propagate_ and not extra_? (propagate sounds like a bool to me)

I think I was trying to communicate that these are headers that propagate from one call to the next. I agree it sounds like a bool.

I guess it's good enough to call them extra_headers.

davidszotten · 2017-07-30T11:03:07Z

nameko/rpc.py

-    @property
-    def serializer(self):
-        """ Name of the serializer to use when publishing response payloads.
+    Publisher = Publisher


i think we tend to name vars like this publisher_cls

davidszotten · 2017-07-30T11:04:38Z

nameko/standalone/events.py

+        )
+    )
+
+    # TODO: standalone event dispatcher should accept context event_data


Wasn't sure whether to add this as part of this PR. It's much easier to do now, and it would bring it in line with the standalone RPC proxy.

davidszotten · 2017-07-30T11:06:08Z

nameko/testing/utils.py

+MockCallArgs = namedtuple("MockCallArgs", ["positional", "keyword"])
+
+
+def unpack_mock_call(call):


do you prefer this to call_args, call_kwargs = call?

I think it's slightly easier to interpret using the named attribute rather than remembering that it unpacks to args, kwargs. If you're only interested in one or the other you don't have to do args, _ = call or whatever.

I'm not wedded to it if you'd rather not introduce a new helper though.

Really I just want Mock's call to be a namedtuple out of the box.

@tonybajan is that an endorsement for call being a namedtuple or for the helper?

for call being a namedtuple – makes so much more sense

i had quick look in the source, and Call already has a bunch of interesting behaviours, e.g. it compares successfully to both a tuple (compared against args) and a dict (matched against kwargz) but nothing obvious to check membership in kwargs like you do

happy to keep this helper if you like it

davidszotten · 2017-07-30T12:57:12Z

also, didn't realise you were still using cagoule! still working ok? given the size of that file, it might be worth re-writing history to remove all traces of it before landing this pr

mattbennett · 2017-08-21T16:26:33Z

@davidszotten I think I've addressed all the comments here. History also rewritten to remove the cagoule file.

I've implemented the TODO on top of this but left it out of this branch. PR of the diff here for easier review: mattbennett#8.

davidszotten

you now have a conflict in changes but otherwise +1

…ublisher-core

mattbennett added 30 commits January 25, 2017 11:59

checkpoint: add delivery and encoding options to publisher class;

e790621

simplify event dispatcher by having it reuse the publisher's implementation

checkpoint: support passing default options into _publishers_ at declare

bc6d99e

time as well as event dispatchers, and test that precedence is correct; adds two new pytest fixtures: amqp_uri and get_message_from_queue (which still need tests)

checkpoint: added behavioural tests for publishing options

3593472

test addding headers; update implementation to avoid clobbering existing

906d592

headers that we want to propagate (e.g. call stack)

add tests for new pytest fixtures

99d0efe

latest setuptools now requires six>=1.10.0

af2ed83

Merge remote-tracking branch 'upstream/master' into connection-delive…

840d8be

…ry-params

typo

8bf70a2

Message expiry requires kombu>=3.0.25

c18c9ce

checkpoint: created Publisher core; DependencyProvider inherits from it

0e97210

checkpoint: using publisher-core in publisher DP and RPC Responder

b9cc1d4

rearranged amqp internals

26c8ce0

checkpoint: tidy up

fb547d3

checkpoint: removed propagation_headers

f0359d4

checkpoint: remove self.queue hack on publisher-core; move to DP

26f3dbb

tidy up refactor of EventDispatcher & Publisher

f668cf8

checkpoint: bring in enhancements from nameko#398/#7

f24395b

checkpoint: bring in enhancements from nameko#398/#7

5ae29a8

add behavioural tests using publisher-core directly

58b948e

add behavioural tests using publisher-core directly

4953cb0

streamline fixture usage

2906d1a

streamline fixture usage

99f23af

tidyup and remove tests that are now done on publisher core

4b69745

tidyup and remove tests that are now done on publisher core

06e4124

remove more tests that are now done against publisher core

f631742

remove more tests that are now done against publisher core

6d11153

move test for declare kwarg into publisher core

0a4d96a

move test for declare kwarg into publisher core

99222d7

finish behavioural tests for publisher core

0862eef

finish behavioural tests for publisher core

c086bf7

mattbennett added 7 commits July 18, 2017 13:49

refactored restricted test because you can't compare a kombu exchange

bf89b7a

to another object that isn't an exchange

add notes to changelog

affa4a1

add notes to changelog

b9d4501

don't check in cagoule.db

44bffa5

don't check in cagoule.db

387c444

s/defaults/options for keyword args

d7b030b

s/defaults/options for keyword args

bf14301

mattbennett requested a review from davidszotten July 20, 2017 16:39

mattbennett mentioned this pull request Jul 20, 2017

Expose delivery options #400

Closed

mattbennett added 4 commits July 20, 2017 18:30

spelling

249760f

spelling

2ced0f2

add non-persistent delivery mode constant

d281a19

add non-persistent delivery mode constant

bd9a3fe

mattbennett mentioned this pull request Jul 28, 2017

Setting and getting message properties manually #374

Closed

davidszotten reviewed Jul 30, 2017

View reviewed changes

mattbennett mentioned this pull request Jul 31, 2017

2.0 event dispatchers should generate headers at dispatch time not at injection time #220

Closed

mattbennett added 6 commits August 18, 2017 15:26

correct version in warning

70b09df

correct version in warning

bed690e

s/propagate_headers/extra_headers

4e1f680

s/propagate_headers/extra_headers

440df74

rename .Publisher to .publisher_cls

19277c0

rename .Publisher to .publisher_cls

b7daf07

mattbennett force-pushed the publisher-core branch from 19277c0 to b7daf07 Compare August 21, 2017 16:23

davidszotten approved these changes Aug 22, 2017

View reviewed changes

mattbennett added 2 commits August 22, 2017 11:33

Merge remote-tracking branch 'upstream/master' into publisher-core

0d2e47b

Merge branch 'publisher-core' of github.com:mattbennett/nameko into p…

9f4db47

…ublisher-core

mattbennett merged commit f09519a into nameko:master Aug 22, 2017

mattbennett mentioned this pull request Aug 22, 2017

Import lint #466

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor AMQP publishing, expose delivery options #456

Refactor AMQP publishing, expose delivery options #456

mattbennett commented Jul 20, 2017

davidszotten left a comment

davidszotten Jul 30, 2017

mattbennett Jul 31, 2017

davidszotten Jul 30, 2017

davidszotten Jul 30, 2017

mattbennett Jul 31, 2017

davidszotten Jul 30, 2017

mattbennett Jul 31, 2017

davidszotten Jul 31, 2017

davidszotten Jul 30, 2017

mattbennett Jul 31, 2017

davidszotten Jul 30, 2017

mattbennett Jul 31, 2017

davidszotten Jul 30, 2017

davidszotten Jul 30, 2017

mattbennett Jul 31, 2017

davidszotten Jul 30, 2017

mattbennett Jul 31, 2017

mattbennett Aug 1, 2017

tonybajan Aug 1, 2017

davidszotten Aug 1, 2017

davidszotten commented Jul 30, 2017

mattbennett commented Aug 21, 2017 •

edited

davidszotten left a comment

		@@ -0,0 +1,4 @@
		# backwards compat imports
		from .utils import verify_amqp_uri # noqa: F401

		MockCallArgs = namedtuple("MockCallArgs", ["positional", "keyword"])


		def unpack_mock_call(call):

Refactor AMQP publishing, expose delivery options #456

Refactor AMQP publishing, expose delivery options #456

Conversation

mattbennett commented Jul 20, 2017

davidszotten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidszotten commented Jul 30, 2017

mattbennett commented Aug 21, 2017 • edited

davidszotten left a comment

Choose a reason for hiding this comment

mattbennett commented Aug 21, 2017 •

edited