Closed Bug 623305 Opened 9 years ago Closed 7 years ago

Switch the python helper library from carrot to kombu

Categories

(Webtools :: Pulse, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: christian, Unassigned)

References

()

Details

Attachments

(1 file, 1 obsolete file)

Shouldn't be too hard as kombu has a carrot-compatible api
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.
Assignee: LegNeato → jgriffin
(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!
Been working on this.
Assignee: jgriffin → mcote
Status: NEW → ASSIGNED
Attached patch Switch from carrot to kombu (obsolete) — Splinter Review
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.
Attachment #758037 - Flags: review?(jgriffin)
Attachment #758037 - Flags: feedback?(dkl)
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 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.
Attachment #758037 - Flags: review?(jgriffin) → review+
Oh, and I forgot to mention, cool!
(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.
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.
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?
Attachment #758037 - Attachment is obsolete: true
Attachment #758037 - Flags: feedback?(dkl)
Attachment #761084 - Flags: review?(dkl)
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 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
Attachment #761084 - Flags: review?(dkl) → review+
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?
Flags: needinfo?(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.
Flags: needinfo?(jgriffin)
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.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
What are the benefits for consumers with the switch to kombu?
(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
(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?
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.
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.
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.
Ouch. Perfect confusion! Sorry for that. We are fine then. :)
You need to log in before you can comment on or make changes to this bug.