praekeltfoundation / vumi-go

BSD 3-Clause "New" or "Revised" License
16 stars 20 forks source link

Improve conversation middleware #1375

Closed KaitCrawford closed 8 years ago

KaitCrawford commented 8 years ago

Prevent the conversation middleware from hitting redis for every message.

KaitCrawford commented 8 years ago

@hodgestar first pass ready for feedback I'm sure I should ping someone else for review but I'm not sure who

KaitCrawford commented 8 years ago

@hodgestar ready for re-review. I'm not sure if it's the right move keeping the conv keys in memory and only adding them to redis later, like I've done now, but I thought it would reduce the likelihood of something weird happening due to the redis set and the local set being cleared at different times.

hodgestar commented 8 years ago

@KaitCrawford Because Twisted is really single threaded it's actually safe to do:

# in looping call
self.local_recent_convs.clear()
...
# later in record_conv_seen
if conv_details not in self.local_recent_convs:
    self.local_recent_convs.add(conv_details)
    return self.redis.sadd(self.RECENT_CONV_KEY, conv_details)
KaitCrawford commented 8 years ago

@hodgestar ready for review again. I'm not sure why the tests are failing since Dummy Request does have an attribute called 'headers'?

hodgestar commented 8 years ago

Left a few more comments. Looks good though.

KaitCrawford commented 8 years ago

@hodgestar comments handled

hodgestar commented 8 years ago

Left one last minor comment. We still need to understand what the test failures are about before this can land though (I'm looking at that a bit now).

hodgestar commented 8 years ago

Test failures are the result of a change in Twisted's DummyRequest object in the new Twisted release (16.0). They're being addressed in #1376.

hodgestar commented 8 years ago

:+1:

(and I've checked that the build failures are unrelated and addressed by #1376)