Rework writing status updates so that updates by external id also use the batcher #617
Conversation
Codecov Report
@@ Coverage Diff @@
## main #617 +/- ##
==========================================
- Coverage 74.06% 73.91% -0.16%
==========================================
Files 96 96
Lines 13099 13132 +33
==========================================
+ Hits 9702 9706 +4
- Misses 2689 2716 +27
- Partials 708 710 +2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -606,19 +606,9 @@ func (ts *BackendTestSuite) TestMsgStatus() { | |||
| ts.Equal(courier.MsgDelivered, m.Status_) | |||
| ts.NotNil(m.SentOn_) | |||
|
|
|||
| // no such external id for outgoing message | |||
| status := ts.b.NewMsgStatusForExternalID(channel, "ext2", courier.MsgSent, clog6) | |||
| err := ts.b.WriteMsgStatus(ctx, status) | |||
There was a problem hiding this comment.
finding out that an external id doesn't exist is now async
| const sqlResolveStatusMsgIDs = ` | ||
| SELECT id, channel_id, external_id | ||
| FROM msgs_msg | ||
| WHERE (channel_id, external_id) IN (VALUES(CAST(:channel_id AS int), :external_id))` |
There was a problem hiding this comment.
note that you can't write :channel_id::int because sqlx.Named(...) will get confused booo
backends/rapidpro/status.go
Outdated
| } | ||
|
|
||
| // find the status with this channel ID and external ID and update its msg ID | ||
| for _, s := range statuses { |
There was a problem hiding this comment.
Can we build a map here and loop on the statuses outside the for loop for rows?
There was a problem hiding this comment.
I guess so.. I went back and forth on that because the map has to use a compound key of channel + external id.. and I persuaded myself that the complexity of creating that isn't worth the performance gains.. but lemme try it.
There was a problem hiding this comment.
Ok it isn't too bad.. updated
This resolves external ids to database ids in the batcher using a database lookup - I think it makes sense to optimize that by stuffing external ids in redis, but also it's not going to be much of a performance hit to not do that for now since the lookup is in batches - and I've checked it's just an index scan on
msgs_by_external_id.Of course the other reason to put external ids in redis is to avoid the race condition of trying to do a status update by external id before the status update that sets the external id. This won't fix that but it should make it less likely because previously the status update to write the external id was being done async (always a delay) but a subsequent update using that external id would by sync (no delay)... now they're both queued, both async, both with a delay.