feat: make gcm calls use async callbacks #1296
Conversation
jrconlin
requested review from
pjenvey and
bbangert
and removed request for
pjenvey
Codecov Report
@@ Coverage Diff @@
## master #1296 +/- ##
==========================================
+ Coverage 99.76% 99.78% +0.02%
==========================================
Files 61 61
Lines 9897 9983 +86
==========================================
+ Hits 9874 9962 +88
+ Misses 23 21 -2
Continue to review full report at Codecov.
|
autopush/router/gcm.py
Outdated
| result = gcm.send(payload) | ||
| except RouterException: | ||
| raise # pragma nocover | ||
| d = gcm.send(payload) |
There was a problem hiding this comment.
What does gcm actually refer to here? We pull a gcm off a gcm?
autopush/router/gcm.py
Outdated
|
|
||
| def _error(self, err, status, **kwargs): | ||
| """Error handler that raises the RouterException""" | ||
| self.log.debug(err, **kwargs) | ||
| return RouterException(err, status_code=status, response_body=err, | ||
| **kwargs) | ||
|
|
||
| def _process_reply(self, reply, uaid_data, ttl, notification): | ||
| def _process_reply(self, gcm, uaid_data, ttl, notification): |
There was a problem hiding this comment.
This gcm is actually whatever the return value is, but above are two more uses of a 'gcm' that mean two other different things, and this gcm is neither of those gcm's. Are these all the same thing? Different things?
|
For now, it should be good enough to validate if deferred processing will reduce CPU lock, and I can implement a better version for the pending FCM changeover. |
autopush/router/gcmclient.py
Outdated
| self._options = options | ||
| self._sender = requests.post | ||
| self._sender = treq.request |
There was a problem hiding this comment.
Heh, I wanted to be explicit rather than rely on a wrapper. Not worth fighting over.
autopush/router/gcmclient.py
Outdated
|
|
||
| def _parse_response(self, message, content): | ||
| def parse_response(self, content, code, message): | ||
| if code in (400, 401, 404): |
There was a problem hiding this comment.
I think the 401 case should be removed from here just to help clarify as it would have already been handled in process()
autopush/router/gcm.py
Outdated
|
|
||
| def _error(self, err, status, **kwargs): | ||
| """Error handler that raises the RouterException""" | ||
| self.log.debug(err, **kwargs) | ||
| return RouterException(err, status_code=status, response_body=err, | ||
| **kwargs) | ||
|
|
||
| def _process_reply(self, reply, uaid_data, ttl, notification): | ||
| def _process_reply(self, client, uaid_data, ttl, notification): |
There was a problem hiding this comment.
why do we get back a client now? (looks like all we need is its response, didn't see it utilized in tests but maybe I missed it)
There was a problem hiding this comment.
You're correct, we only really need client.response. It just felt odd to me that we were making a request of GCM and returning something else. Not a big deal, reverting.
autopush/web/webpush.py
Outdated
| @@ -527,6 +527,9 @@ def _router_completed(self, response, uaid_data, warning="", | |||
| router_type=None, vapid=None): | |||
| """Called after router has completed successfully""" | |||
| # Log the time taken for routing | |||
| print("router_complete response: ", response) | |||
There was a problem hiding this comment.
looks good except is this leftover debugging?
autopush/web/webpush.py
Outdated
| @@ -527,6 +527,8 @@ def _router_completed(self, response, uaid_data, warning="", | |||
| router_type=None, vapid=None): | |||
| """Called after router has completed successfully""" | |||
| # Log the time taken for routing | |||
| if response is None: | |||
There was a problem hiding this comment.
is this needed as well? I figured it was also stray debugging, seems like route_notification still always returns something
There was a problem hiding this comment.
I had originally included it because the router is technically a module that may fail. I might revisit this in the future, since you're right that we should still capture metric info off of it, but for now, it's unused code.
Closes #1291