Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Nameko 2.11.0 and v3 prerelease #26

Merged
merged 10 commits into from Nov 23, 2018
Merged

Support Nameko 2.11.0 and v3 prerelease #26

merged 10 commits into from Nov 23, 2018

Conversation

mattbennett
Copy link
Member

@mattbennett mattbennett commented Nov 20, 2018

Changes:

  • Use the Publisher from nameko.amqp.publish in the backoff publisher
  • Remove the retry mechanism in the publisher, because Kombu 4 doesn't raise multiple times
  • Test on Python > 3.4

Delgating to the Publisher utility means we can run against multiple versions of Nameko and whatever versions of Kombu that it chooses.

Additional changes for compatibility with the 3.x prerelease (3a10df6):

  • ack_message has moved from queue_consumer to consumer, so a try/except is added
  • the call stack format has changed slightly, so a conditional is added

@mattbennett mattbennett mentioned this pull request Nov 20, 2018
@@ -124,44 +122,33 @@ def republish(self, backoff_exc, message, target_queue):
expiration = backoff_exc.next(message, self.exchange.name)
queue = self.make_queue(expiration)

# republish to appropriate backoff queue
properties = message.properties.copy()
headers = properties.pop('application_headers')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had en issue where in some cases (sorry, can't remember what) that property was not set. Providing a default value (empty dict) would fix it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I will fix this 👍

@cblegare
Copy link

Thank you for this.

As discussed in #25, I still have issues with some tests. I'll post examples here in case someone reproduce it

This one happens randomly on any version

____ TestMessaging.test_multiple_queues_with_same_exchange_and_routing_key _____
<snip>
>       assert backoff_queue['messages'] == 0
E       KeyError: 'messages'

This one only happened in py27-oldest-lib

__________________________ TestPublisher.test_routing __________________________
<snip>
>       assert backoff_queue['messages'] == delays.count(delay)
E       assert 0 == 1
E        +  where 1 = <built-in method count of list object at 0x7f7ced4f00e0>(10000)
E        +    where <built-in method count of list object at 0x7f7ced4f00e0> = [10000, 20000, 20000, 30000, 30000, 30000].count

test/test_retry.py:75: AssertionError

@mattbennett
Copy link
Member Author

mattbennett commented Nov 20, 2018

I can reliably reproduce the KeyError: 'messages' on RabbitMQ 3.6.7 and above. That release introduced the "distributed management plugin" which caused problems with the Nameko test suite too. From that release onwards changes made through the REST API are only eventually consistent. I worked around it in the Nameko testsuite by querying the state of the queues using AMQP instead. I can do that here too I think.

Is your other error consistent or sporadic?

@cblegare
Copy link

Sporadic (roughly 60% repro). This would be consistent with plugin change you mentionned

@mattbennett
Copy link
Member Author

Great. I think all of these errors are explained by this change in the HTTP API. Inspecting the queues with AMQP seems to work :)

test against more modern rabbitmq
@cblegare
Copy link

Good!

Just saying, would it be a good idea to make the rabbit_manager use both http and amqp?

Thank you a lot for your work.

@mattbennett
Copy link
Member Author

@cblegare rabbit_manager is just a thin wrapper around (some parts of) the REST API. I'd rather keep it simple than make just the queue inspection part of it use AMQP.

My colleague @kooba pointed out that this is branch is still not compatible with the pre-release versions of Nameko (3.x), so I will bundle that into this branch

@mattbennett mattbennett changed the title Support Nameko 2.11.0 Support Nameko 2.11.0 and v3 prerelease Nov 22, 2018
@mattbennett
Copy link
Member Author

The prerelease tests here will fail until the changes in nameko/nameko#592 are released

@cblegare
Copy link

@mattbennett Thank you! I'll look into nameko 3 too

Copy link
Contributor

@iky iky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@mattbennett mattbennett merged commit 67f3e42 into master Nov 23, 2018
@mattbennett mattbennett deleted the upgrade-kombu branch November 23, 2018 09:57
This was referenced Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants