Make kannel channel paused when we get Queued message in response #670
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #670 +/- ##
==========================================
+ Coverage 74.38% 74.41% +0.03%
==========================================
Files 97 97
Lines 13082 13091 +9
==========================================
+ Hits 9731 9742 +11
+ Misses 2662 2660 -2
Partials 689 689 ☔ View full report in Codecov by Sentry. |
handlers/kannel/handler.go
Outdated
| rc.Do("SET", rateLimitKey, "engaged") | ||
|
|
||
| // We pause sending 30 seconds so the connection to the SMSC is reset | ||
| rc.Do("EXPIRE", rateLimitKey, 30) |
There was a problem hiding this comment.
SET + EXPIRE = SETNX.. no ?
There was a problem hiding this comment.
That is deprecated https://redis.io/commands/setnx/
But I can do on line as rc.Do("SET", rateLimitKey, "engaged", "EX", 30)
| if strings.Contains(string(respBody), "Queued") { | ||
| rc := h.Backend().RedisPool().Get() | ||
| defer rc.Close() | ||
| rateLimitKey := fmt.Sprintf("rate_limit:%s", msg.Channel().UUID()) |
There was a problem hiding this comment.
It doesn't feel ideal having handlers talk directly to Redis to rate limit themselves and I see we only currently do this in the old WhatsApp onprem handler. And there's rate_limit_bulk.. don't we need to rate limit on both queues?
Maybe we need to have a think about making a cleaner way for channels to rate limit themselves?
There was a problem hiding this comment.
Okay, we can find a new way tor that.
I do not think we need to pause the bulk queue in this case but I will double check on that again
There was a problem hiding this comment.
Added pausing the bulk queue as well
norkans7
force-pushed
the
kannel-pausing
branch
2 times, most recently
from
698d6c1
to
f1227b1
Compare
norkans7
force-pushed
the
kannel-pausing
branch
from
f1227b1
to
e8096c0
Compare
No description provided.