Closed Bug 1287537 Opened 8 years ago Closed 7 years ago

Create service for consuming pulse messages in MozReview

Categories

(MozReview Graveyard :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: smacleod, Assigned: smacleod)

References

Details

Attachments

(3 files, 2 obsolete files)

Currently MozReview can send pulse messages but has no generic service for receiving data over pulse. This will be needed for both autoland's refactor to use pulse and receiving results from automation on review requests.
Blocks: 1287538
This probably lives on a separate EC2 instance somewhere, like Autoland. If we have to talk to a local database instead of e.g. use the MozReview HTTP APIs, we've failed because that means other people can't deploy their own bots to post to MozReview.
Blocks: 1290249
Comment on attachment 8805973 [details]
mozreviewpulse: create package for mozreview's pulse listener (Bug 1287537).

Hey, could you take a high level peek at this and let me know if you like the direction this took. I'm working on the other parts required to get the analysis results into MozReview. Tests will also come when this is less WIP.
Attachment #8805973 - Flags: feedback?(glob)
Because I like to challenge assumptions, I'll throw out an alternative: publish events to an Amazon SNS topic and have Amazon Lambda react to events published on that topic.

An advantage of this model is you won't need to run a server to consume events. Instead, Lambda handles that for you. The downside is you'd need to publish MozReview changes to Amazon SNS and we'd have N+1 notification mechanisms. This does undermine Pulse somewhat.

I'm not suggesting we do this - just throwing out a competing idea [that doesn't require a single-homed service for subscribing to Pulse].
(In reply to Gregory Szorc [:gps] from comment #8)
> Because I like to challenge assumptions, I'll throw out an alternative:
> publish events to an Amazon SNS topic and have Amazon Lambda react to events
> published on that topic.
> 
> An advantage of this model is you won't need to run a server to consume
> events. Instead, Lambda handles that for you. The downside is you'd need to
> publish MozReview changes to Amazon SNS and we'd have N+1 notification
> mechanisms. This does undermine Pulse somewhat.
> 
> I'm not suggesting we do this - just throwing out a competing idea [that
> doesn't require a single-homed service for subscribing to Pulse].

Yup, it's an interesting idea. I was actually considering something similar in the past - Taskcluster has a "hooks" service, where the idea is you can setup tasks which will be automatically run in response to events. I've been told that the plan is to add support for pulse messages being a triggering event, but sadly only cron style recurring tasks are supported at this time.
Comment on attachment 8805973 [details]
mozreviewpulse: create package for mozreview's pulse listener (Bug 1287537).

as per our discussion this looks like it's on the right path.
Attachment #8805973 - Flags: feedback?(glob) → feedback+
Attachment #8805973 - Attachment is obsolete: true
Attachment #8805977 - Attachment is obsolete: true
Comment on attachment 8805974 [details]
mozreviewpulse: add a FIFO pulse consumer (Bug 1287537).

https://reviewboard.mozilla.org/r/89564/#review100872

::: mozreviewpulse/mozreviewpulse/consumer.py:1
(Diff revision 2)
> +import logging

MPL?

::: mozreviewpulse/mozreviewpulse/consumer.py:35
(Diff revision 2)
> +    ordering. Instead the message should not be acked
> +    or nacked, which will cause it to be processed again
> +    as previously mentioned.
> +    """
> +
> +    def __init__(self, host, port, userid=None, password=None,

Can you add a docstring explaining the parameter values?  For instance, what is a userid's value, type, and source?  Is timeout in seconds?  Is ssl a boolean?  etc.

::: mozreviewpulse/mozreviewpulse/consumer.py:50
(Diff revision 2)
> +        self.exchange = exchange
> +        self.queue = queue
> +        self.routing_key = routing_key
> +        self.callback = callback
> +
> +        self.logger = logger or logging.getLogger('mozreviewpulse')

I'm used to seeing loggers defined at the module level:

```
log = logging.getLogger(__name__)
```

Overriding the object's logger with an arg is fine for testability, but initialing a logger in an object with a hard-coded name is a bit odd.

The logger name also looks rather generic, almost package-level.  Maybe it should be imported into all package modules as mozreviewpulse.log or something?

::: mozreviewpulse/mozreviewpulse/consumer.py:130
(Diff revision 2)
> +        """Consume pulse messages.
> +
> +        If a `limit` is provided a `limit` number of messages will be
> +        consumed or consumption will stop when the queue is empty,
> +        whichever comes first. Otherwise, consumption will continue
> +        indefinitely.

Does 'indefinitely' mean that messages will be consumed until the queue is empty, or that the call will block until more messages come in?

::: mozreviewpulse/tests/conftest.py:83
(Diff revision 2)
>          max_retries=10, interval_start=0.3, interval_step=0.3)
>      return conn
> +
> +
> +@pytest.fixture
> +def pulse_producer(request, pulse_conn):

lint? `request` is unused

::: mozreviewpulse/tests/conftest.py:84
(Diff revision 2)
>      return conn
> +
> +
> +@pytest.fixture
> +def pulse_producer(request, pulse_conn):
> +    exchange = kombu.Exchange('exchange/mrp/', type='topic')

are there any explicit cleanup actions necessary on the consumer, exchange, or producer?

::: mozreviewpulse/tests/conftest.py:87
(Diff revision 2)
> +@pytest.fixture
> +def pulse_producer(request, pulse_conn):
> +    exchange = kombu.Exchange('exchange/mrp/', type='topic')
> +    producer = kombu.Producer(pulse_conn, exchange=exchange)
> +
> +    # Esnure the exchange is declared so that consumers

typo
Attachment #8805974 - Flags: review?(mars) → review-
Comment on attachment 8805974 [details]
mozreviewpulse: add a FIFO pulse consumer (Bug 1287537).

https://reviewboard.mozilla.org/r/89564/#review101108

Banking an r+ for later, feel free to land this after the issues are addressed.
Attachment #8805974 - Flags: review- → review+
Comment on attachment 8805975 [details]
mozreviewpulse: absorb needed mozreviewbots libraries and delete the rest (Bug 1287537).

https://reviewboard.mozilla.org/r/89566/#review101422

LGTM
Attachment #8805975 - Flags: review?(mars) → review+
Comment on attachment 8805976 [details]
mozreviewpulse: cleanup batchreview style (Bug 1287537).

https://reviewboard.mozilla.org/r/89568/#review101424

LGTM
Attachment #8805976 - Flags: review?(mars) → review+
Pushed by smacleod@mozilla.com:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/0acfc2c6fd8e
mozreviewpulse: add a FIFO pulse consumer . r=mars
https://hg.mozilla.org/hgcustom/version-control-tools/rev/11fb4626f5c3
mozreviewpulse: absorb needed mozreviewbots libraries and delete the rest . r=mars
https://hg.mozilla.org/hgcustom/version-control-tools/rev/8eb831967434
mozreviewpulse: cleanup batchreview style . r=mars
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: