Closed Bug 980330 Opened 9 years ago Closed 9 years ago

[pulsebuildmonitor] Change wording from "Connecting to" to "Connected to" when connection has been established

Categories

(Webtools :: Pulse, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: cosmin-malutan, Assigned: cosmin-malutan)

References

()

Details

Attachments

(2 files, 2 obsolete files)

As discussed in issue #410(https://github.com/mozilla/mozmill-ci/issues/410),
we want to change the wording from "Connecting to" to "Connected to" in pulsebuildmonitor
Attached patch patch_v1.0 (obsolete) — Splinter Review
Here is the patch. I only touched the log message.
Assignee: nobody → cosmin.malutan
Attachment #8386785 - Flags: review?(hskupin)
Not sure why you are still using this bugzilla component. You are aware that mozmill-automation is hosted on github those days.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → INVALID
Well, a second look showed me that this bug is not about automation but about pulsebuildmonitor. So please make sure to really file the bug into the correct component the next time.
Status: RESOLVED → REOPENED
Component: Mozmill Automation → Pulse
Product: Mozilla QA → Webtools
QA Contact: hskupin
Resolution: INVALID → ---
Summary: Change the wording from "Connecting to" to "Connected to" in pulsebuildmonitor → [pulsebuildmonitor] Change wording from "Connecting to" to "Connected to" when connection has been established
Version: unspecified → other
Comment on attachment 8386785 [details] [diff] [review]
patch_v1.0

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

So we cannot simply change this logger statement given that it will be actually printed before the connection happens. So what you want to is to add another logger statement after the connection has been established. Something like 'Connected' should be totally enough.
Attachment #8386785 - Flags: review?(hskupin) → review-
Attached patch patch_v2.0 (obsolete) — Splinter Review
I added a log after the connection has been established, 
The output looks like:
>./pulse.py config/staging/l10n.json 
>INFO:automation:Connecting to Mozilla Pulse as "qa-auto@mozilla.com|mozmill_l10n|P5139"...
>INFO:automation:Connected
Attachment #8386785 - Attachment is obsolete: true
Attachment #8387465 - Flags: review?(hskupin)
Comment on attachment 8387465 [details] [diff] [review]
patch_v2.0

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

Looks fine to me.
Attachment #8387465 - Flags: review?(hskupin) → review+
Comment on attachment 8387465 [details] [diff] [review]
patch_v2.0

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

Given that I'm not an official peer of the package, Jonathan may have to finish this off and land it.
Attachment #8387465 - Flags: review?(jgriffin)
Status: REOPENED → ASSIGNED
Comment on attachment 8387465 [details] [diff] [review]
patch_v2.0

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

Unfortunately, this isn't as easy as it looks.  We don't actually perform the connection to pulse until pulse.listen() is called.  But, that call is blocking, and won't return until an exception is thrown (due to CTRL+C, network error, etc).

The connection doesn't occur until this line is called:  http://hg.mozilla.org/automation/mozillapulse/file/f5fb7628a206/mozillapulse/consumers.py#l131

You could add an optional callback argument to listen() that would be called after a successful connection, or you could add logger support to mozillapulse.
Attachment #8387465 - Flags: review?(jgriffin) → review-
Oh, you are right. I feel bad by missing that critical piece. Thanks Jonathan! Also I agree, an onconnection callback sounds like a good idea here, which will help consumers of Mozilla Pulse to handle their own connection code.
To add an extra callback will require an update in mozillapulse too. 
I would log the message in already existing callback, pulse_message_received, and set a flag so it will log it only once.
http://hg.mozilla.org/automation/pulsebuildmonitor/file/e64ccc6cc647/pulsebuildmonitor/pulsebuildmonitor.py#l131 
Henrik, what do you think? Can I add the log in the existing method, or add a new callback, and modify the mozillapulse too?
Flags: needinfo?(hskupin)
(In reply to Cosmin Malutan from comment #10)
> To add an extra callback will require an update in mozillapulse too. 

Have you read the two comments above from Jonathan and myself? You would have seen that no change of mozillapulse is necessary here. What you have to modify is the listen() method of pulsebuildmonitor:

http://hg.mozilla.org/automation/mozillapulse/file/f5fb7628a206/mozillapulse/consumers.py#l112
Flags: needinfo?(hskupin)
I have read that and the code also, but listen method is in mozillapulse tool. I cannot just call the listen method with an callback argument, this will overwrite the existing callback set in  __init__ method.
http://hg.mozilla.org/automation/mozillapulse/file/f5fb7628a206/mozillapulse/consumers.py#l40

I will prepare two patches one for mozillapulse(where listen method is defined) and one in pulsebuildmonitor where the method is called.
Ups, and I failed again. Sounds good. I think I will leave anything else on this bug for Jgriffin. :)
Jonathan, if I will add multiple callbacks here:
http://hg.mozilla.org/automation/mozillapulse/file/f5fb7628a206/mozillapulse/consumers.py#l131
Do I have to acknowledge the message in each callback or is sufficient in one callback?
http://ask.github.io/kombu/reference/kombu.mixins.html#kombu.mixins.ConsumerMixin
Flags: needinfo?(jgriffin)
For the purposes of this back, you don't want to add another callback to http://hg.mozilla.org/automation/mozillapulse/file/f5fb7628a206/mozillapulse/consumers.py#l131; that list of callbacks is called whenever a pulse message is received.

Instead, you want to create a new type of callback, maybe something like:

def listen(self, callback, on_connect_callback=None):

# ... a bunch of existing code ...

# Create a queue and bind to the first key.
queue = self._create_queue(self.applabel, exchange, self.topic[0])
if on_connect_callback:
    on_connect_callback()
consumer = self.connection.Consumer(queue, callbacks=[self.callback])
Flags: needinfo?(jgriffin)
Thanks Jonathan, this is patch for mozillapulse, I will add one for pulsebuildmonitor too.
Attachment #8387465 - Attachment is obsolete: true
Attachment #8396424 - Flags: review?(jgriffin)
This is the patch for pulsebuildmonitor, should be landed if all is good only after mozillapulse.
Attachment #8396428 - Flags: review?(jgriffin)
Attachment #8396428 - Flags: review?(jgriffin) → review+
Attachment #8396424 - Flags: review?(jgriffin) → review+
Not sure if you can land these; if not, I'll do it.
Cosmin doesn't have permissions yet. So yes, please land those. Thanks.
For closing this we have to wait for a new version of mozilla pulse and the for a new version of pulsebuildmonitor.
Jhonathan do you know when it will be the next mozillapulse release?
Flags: needinfo?(jgriffin)
Thanks Jonathan, we will have just to change the pulsebuildmonitor dependency version in mozmill-ci.
The output is:
>(jenkins-env)cosmin@P5139:~/Projects/Mozilla/mozmill-ci$ ./pulse.py config/production/daily.json 
>2014-04-09T09:11:29Z INFO:automation:Connecting to Mozilla Pulse as "qa-auto@mozilla.com|mozmill_daily|P5139"...
>2014-04-09T09:11:29Z INFO:automation:Connected
I close this as fixed.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.