Switch to new message store #1310
Conversation
| @@ -152,7 +152,9 @@ def send_scheduled_messages(self, conv): | |||
| def send_message(self, batch_id, to_addr, content, msg_options): | |||
| msg = yield self.send_to( | |||
| to_addr, content, endpoint='default', **msg_options) | |||
| yield self.vumi_api.mdb.add_outbound_message(msg, batch_ids=[batch_id]) | |||
| # XXX: Do we need to do this? | |||
There was a problem hiding this comment.
Probably not given that we now have middleware that knows how to store messages for conversations. The batch_id parameter can likely go too. Perhaps we should open an issue for this for later?
There was a problem hiding this comment.
I created #1322 for this.
|
Left some comments but generally looks good to me. |
|
lgtm too |
| @@ -354,9 +353,10 @@ def _update_tag_data_for_acquire(self, user_account, tag): | |||
| # The batch we create here gets added to the tag_info and we can fish | |||
| # it out later. When we replace this with proper channel objects we can | |||
| # stash it there like we do with conversations and routers. | |||
| yield self.api.mdb.batch_start([tag], user_account=user_account.key) | |||
| yield self.api.get_batch_manager().batch_start( | |||
There was a problem hiding this comment.
Minor thing... maybe put the batch manager in a variable here since get_batch_manager() creates a new manager (although they're pretty lightweight).
|
👍 from me |
|
👍 from me although I think someone with a bit more vumi-go knowledge needs to read over 654cf8e |
|
I left some comments about the message rate counting, but otherwise looks good. |
Old code that I forgot to remove. Thanks for picking that up. (I was initially attempting to match the behaviour of the previous implementation, which started counting from the most recent message rather than the current time.)
The default index page is 1k results. When we were using the batch info cache, we stored up to 2k keys. The effect of having more keys in the window than we can fit in the page is the same in both cases, but the threshold is a little different. This information should really come from metrics rather than message store lookups, but that's out of scope for this PR. |
|
Cool. I think if we just document the 1k limit here that's good enough for
now.
|
|
👍 from me on what's here currently. |
No description provided.