Closed
Bug 623305
Opened 15 years ago
Closed 12 years ago
Switch the python helper library from carrot to kombu
Categories
(Webtools :: Pulse, enhancement)
Webtools
Pulse
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: christian, Unassigned)
References
()
Details
Attachments
(1 file, 1 obsolete file)
28.33 KB,
patch
|
dkl
:
review+
|
Details | Diff | Splinter Review |
Shouldn't be too hard as kombu has a carrot-compatible api
![]() |
||
Comment 1•14 years ago
|
||
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.
![]() |
||
Updated•14 years ago
|
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!
![]() |
Assignee | |
Comment 3•12 years ago
|
||
Been working on this.
Assignee: jgriffin → mcote
Status: NEW → ASSIGNED
![]() |
Assignee | |
Comment 4•12 years ago
|
||
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)
![]() |
Assignee | |
Comment 5•12 years ago
|
||
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•12 years ago
|
||
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+
![]() |
||
Comment 7•12 years ago
|
||
Oh, and I forgot to mention, cool!
![]() |
Assignee | |
Comment 8•12 years ago
|
||
(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.
![]() |
Assignee | |
Comment 9•12 years ago
|
||
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.
![]() |
Assignee | |
Comment 10•12 years ago
|
||
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)
![]() |
Assignee | |
Comment 11•12 years ago
|
||
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•12 years ago
|
||
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+
![]() |
Assignee | |
Comment 13•12 years ago
|
||
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)
![]() |
||
Comment 14•12 years ago
|
||
(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)
![]() |
Assignee | |
Comment 15•12 years ago
|
||
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.
![]() |
Assignee | |
Updated•12 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 16•12 years ago
|
||
What are the benefits for consumers with the switch to kombu?
Comment 17•12 years ago
|
||
(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•12 years ago
|
||
(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?
![]() |
Assignee | |
Comment 19•12 years ago
|
||
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•12 years ago
|
||
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.
![]() |
Assignee | |
Comment 21•12 years ago
|
||
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•12 years ago
|
||
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.
Description
•