623305 - Switch the python helper library from carrot to kombu

Status: RESOLVED FIXED

(Webtools :: Pulse, enhancement)

Description

[LegNeato]

Shouldn't be too hard as kombu has a carrot-compatible api

Comment 1

[Jonathan Griffin (:jgriffin)]

We may also want to investigate using pika in lieu of amqplib, as the former supports heartbeats which can help to prevent stale connections from hanging around forever, among other things.

Comment 2

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to Jonathan Griffin (:jgriffin) from comment #1)
> We may also want to investigate using pika in lieu of amqplib, as the former
> supports heartbeats which can help to prevent stale connections from hanging
> around forever, among other things.

Heartily agreed!

Comment 3

[Mark Côté [:mcote]]

Been working on this.

Comment 4

[Mark Côté [:mcote]]

Attachment: Switch from carrot to kombu

I tested this locally, both with the accompanying (and, in this patch, expanded) tests, with the simple test setup described in the README, and with the pulsebuildmonitor and logparser.  All work fine.  I've done limited publisher testing, though, as that is harder to test.  jgriffin, if you have a test version of one of the publishers, can you give this a whirl?  The harder part was the consumer, so I'm not worried, but I want to be reasonably sure I didn't break anything before deploying.

I also bumped the version and set myself as package owner in lieu of Legneato.

Comment 5

[Mark Côté [:mcote]]

Update: I set up a modified pulsetranslator that listens to pulse but publishes normalized messages to my local rabbitmq instance, and hooked up a modified pulsebuildmonitor to that, and things seemed to work perfectly (once I figured out what I was doing. :)

Comment 6

[Jonathan Griffin (:jgriffin)]

Comment on attachment 758037 [details] [diff] [review]
Switch from carrot to kombu

Review of attachment 758037 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozillapulse/consumers.py
@@ +138,5 @@
> +            hb_exchange = Exchange('org.mozilla.exchange.pulse.test',
> +                                   type='topic')
> +            consumer.queues[0].bind_to(hb_exchange, 'heartbeat')
> +
> +        with consumer:

I don't think this line is necessary, is it?

::: setup.py
@@ +7,5 @@
>  
>  from setuptools import setup
>  
>  setup(name='MozillaPulse',
> +      version='0.7',

Should actually be 0.70, or 0.62, or whatever.  0.7 < 0.61 according to distutils.

Comment 7

[Jonathan Griffin (:jgriffin)]

Oh, and I forgot to mention, cool!

Comment 8

[Mark Côté [:mcote]]

(In reply to Jonathan Griffin (:jgriffin) from comment #6)
> Comment on attachment 758037 [details] [diff] [review]
> Switch from carrot to kombu
> 
> Review of attachment 758037 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mozillapulse/consumers.py
> @@ +138,5 @@
> > +            hb_exchange = Exchange('org.mozilla.exchange.pulse.test',
> > +                                   type='topic')
> > +            consumer.queues[0].bind_to(hb_exchange, 'heartbeat')
> > +
> > +        with consumer:
> 
> I don't think this line is necessary, is it?

It isn't strictly necessary, but it ensures that consume() and cancel() are called (via __enter__() and __exit__()).  In particular, it means that cancel() will be called even if the drain_events() calls raises an exception.  Pretty much all the kombu examples that I've seen use this "with" pattern.  I can add a comment explaining it, though.

> 
> ::: setup.py
> @@ +7,5 @@
> >  
> >  from setuptools import setup
> >  
> >  setup(name='MozillaPulse',
> > +      version='0.7',
> 
> Should actually be 0.70, or 0.62, or whatever.  0.7 < 0.61 according to
> distutils.

Right you are.

Comment 9

[Mark Côté [:mcote]]

dkl reported a problem with this patch on his linux system. I reproed the problem on my Linux system as well (having only tested it under OSX previously. I'll investigate; at the moment all I know is that the consumer doesn't seem to be seeing any messages on its queue.

Comment 10

[Mark Côté [:mcote]]

Attachment: Switch from carrot to kombu, v2

I think this was just a problem with the test, misusing queues through terminating processes.  I'm not sure why it didn't happen with carrot, though I suspect it's just timing.

dkl can you verify that this works for you?

Comment 11

[Mark Côté [:mcote]]

Ah actually it does happen sometimes with carrot; it's just that this patch also added tests, and the bug was in there, not in the mozillapulse code itself.

Comment 12

[David Lawrence [:dkl]]

Comment on attachment 761084 [details] [diff] [review]
Switch from carrot to kombu, v2

Once I remembered to change the host to localhost in the test script, all was well :)

[vagrant@rabbitmq-server1 mozillapulse]$ python test/runtests.py
....
----------------------------------------------------------------------
Ran 4 tests in 0.636s

OK

Comment 13

[Mark Côté [:mcote]]

https://hg.mozilla.org/automation/mozillapulse/rev/2d0ca2d4e2b8

jgriffin, can you add me to the pypi owners list for mozillapulse so I can publish v0.70?

Comment 14

[Jonathan Griffin (:jgriffin)]

(In reply to Mark Côté ( :mcote ) from comment #13)
> https://hg.mozilla.org/automation/mozillapulse/rev/2d0ca2d4e2b8
> 
> jgriffin, can you add me to the pypi owners list for mozillapulse so I can
> publish v0.70?

Done.

Comment 15

[Mark Côté [:mcote]]

Forgot the version bump: https://hg.mozilla.org/automation/mozillapulse/rev/24973ce288e0

0.70 has been uploaded to pypi.  We can now proceed to upgrade the consumers & producers.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

What are the benefits for consumers with the switch to kombu?

Comment 17

[David Lawrence [:dkl]]

(In reply to Henrik Skupin (:whimboo) from comment #16)
> What are the benefits for consumers with the switch to kombu?

The Carrot lib that mozillapulse was originally based off of is no longer being maintained and the Carrot developers themselves are recommending for developers to
switch to Kombu which is mostly compatible.

dkl

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Mark Côté ( :mcote ) from comment #15)
> 0.70 has been uploaded to pypi.  We can now proceed to upgrade the consumers
> & producers.

Err. Version 0.70 has already been uploaded in February this year. So most likely we want to release 0.8?

Comment 19

[Mark Côté [:mcote]]

Not according to hg.  The revision history shows a bump to 0.6 on March 21, 2012, (https://hg.mozilla.org/automation/mozillapulse/rev/4673f153d891), then a bump to 0.61 on April 4, 2012, (https://hg.mozilla.org/automation/mozillapulse/rev/d08b0eeb637d), and no more changes until May 31, 2013, when I added tests.  Version was bumped to 0.70 just after I committed the carrot -> kombu change.  0.61 was also the most recently listed version on pypi until I uploaded 0.70 yesterday.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

We are installing pulsebuildmonitor 0.70 for a long time already in our mozmill-ci project. See the archive listing for upload dates:

https://pypi.python.org/packages/source/p/pulsebuildmonitor/

I don't see any package uploaded yesterday.

Comment 21

[Mark Côté [:mcote]]

This bug is about the mozillapulse library, not pulsebuildmonitor. :) My point in comment #15 is that all users of the mozillapulse library, including pulsebuildmonitor instances, probably should start using the new mozillapulse library at some point.  They can then keep up with kombu improvements and fixes, since carrot is dead.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ouch. Perfect confusion! Sorry for that. We are fine then. :)