Introduce MessageQueue #537
Conversation
|
Fucking awesome! |
jh0ker
left a comment
jh0ker
left a comment
There was a problem hiding this comment.
This looks pretty solid to me already, however there are a few things I'd like to see cleared up before we can merge.
There are also a few mentions of dispatcher or dispatching which might be confused with telegram.ext.Dispatcher. Can those be replaced with something more clearly referring to MessageQueue without changing the meaning?
telegram/ext/messagequeue.py
Outdated
| @@ -0,0 +1,309 @@ | |||
| '''A throughput-limiting message dispatcher for Telegram bots''' | |||
There was a problem hiding this comment.
Please add the default header we have in all source files
telegram/ext/messagequeue.py
Outdated
| queue (:obj:`queue.Queue`, optional): used to pass callbacks to | ||
| thread. | ||
| Creates `queue.Queue` implicitly if not provided. | ||
| burst_limit (:obj:`int`, optional): numer of maximum callbacks to |
| time_limit_ms (:obj:`int`, optional): defines width of time-window | ||
| used when each dispatching limit is calculated. | ||
| Defaults to 1000. | ||
| exc_route (:obj:`callable`, optional): a callable, accepting 1 |
There was a problem hiding this comment.
I thought we were relying on the promise here? Although it might still make sense to have both... just wondering if this is intentional.
There was a problem hiding this comment.
Promises are used, but DelayQueue is left generic because it may be useful outside of the throughput-limiting scope
| queued (:obj:`bool`, optional): if set to ``True``, the `MessageQueue` | ||
| is used to dispatch output messages. | ||
| Defaults to `self._is_queued_out`. | ||
| isgroup (:obj:`bool`, optional): if set to ``True``, the message is |
There was a problem hiding this comment.
We could at least guess this by using the chat id (negative chat_id usually means it's a group). I know it's an implementation detail, but I'd still consider it a sane default in case this is not specified.
There was a problem hiding this comment.
Rejected. An accurate approach (a cached chat_id:type mapping) would be introduced further
telegram/ext/messagequeue.py
Outdated
|
|
||
| @functools.wraps(method) | ||
| def wrapped(self, *args, **kwargs): | ||
| queued = kwargs.pop('queued', self._is_queued_out) |
There was a problem hiding this comment.
Just to make sure I understand this correctly, _is_queued_out will be an attribute of telegram.Bot? In that case, I'd prefer a name that includes the word default, like _queue_messages_default (or maybe something more concise?)
There was a problem hiding this comment.
Done. Changed to _is_messages_queued_default. In my opinion, is should be left because it's common practice if attribute or variable is boolean
| if len(times) >= self.burst_limit: # if throughput limit was hit | ||
| time.sleep(times[1] - t_delta) | ||
| # finally dispatch one | ||
| try: |
There was a problem hiding this comment.
See my comment about promises earlier: item would be a telegram.util.promise.Promise object, right?
There was a problem hiding this comment.
item is a tuple of form (func, args, kwargs) which becomes (promise, (), {}) and (delayqueue.__call__, (promise, ), {}) for corresponding processors
|
Travis shows errors with |
|
As it may be seen, Travis shows yapf error only on Python 3.5. |
|
@thodnev We only run pre-commit hooks on python3.5 build, so we can save some time and easily see if the tests fail or if it's only the hooks that fail. |
This commit is 2nd step on bringing output messages throughput-limiting mechanisms to the lib.
I've briefly mentioned the ideas behind
MessageQueuein dev chat before. Here are the main points of the commit:telegram.ext.messagequeuemodule, containingDelayQueue,MessageQueueclasses andqueuedmessagedecorator;DelayQueueis the core of throughput-limiting mechanism. It runsPromisecallbacks, dispatched by queue in a thread, with respect to provided limits;MessageQueueis a class, which embeds two instances ofDelayQueuein sequential-interconnection way (one for group messages, another for all messages). The figure below illustrates how it works:queuedmessagedecorator is meant to be used withtelegram.bot.Botoutput conversation methods to make them accept additional kwargs. Messages are passed throughMessageQueuedepending on kwargs set;telegram.ext.messagequeuemodule is properly documented for good usability;DelayQueue) is added.So, the next step in the process would be to couple
MessageQueuewithtelegram.bot.Botin best way ;)