Provides a working SMS implementation with support for Twilio.
This version doesn't really retry if we get any gruff at all. Future versions should retry.
Differential D8930
Add SMS support btrahan on May 1 2014, 6:24 PM. Authored by Tags None Referenced Files
Subscribers Tokens
Details
Provides a working SMS implementation with support for Twilio. This version doesn't really retry if we get any gruff at all. Future versions should retry. used bin/sms to send messages and look at them.
Diff Detail
Event Timeline
Comment Actions
Twilio is the only one I'm actively aware of / recognize the brand name of. We might want a "drop in a hole" adapter for testing, but I don't think we need to support anything else out of the gate. Looks like there are a ton of them in general, though. I don't recognize any of these names other than Twilio (and the major carriers) offhand: http://blog.mashape.com/post/56272188360/list-of-50-sms-apis
I don't think so -- I'd guess that (like MetaMTA) all the info ends up being too sensitive to put on the web. I'd probably shove the code in infrastructure/ or metamta/ and then make a CLI bin/sms to provide a list, a send-test, etc.
Nope, this looks great to me.
I'd probably just guess / go with your gut, it should be pretty easy to update this table later. We can GC it after 90 days or something like we do with mail, so it won't be too big.
Comment Actions NOTE: see Daemons section below if you have time. That's the big juicy stuff you could help me with.
CAVEAT - this is in the "almost done for review" phase, which is to say I am testing it and it isn't working and I am confused but figure it can't take long now...! I need to cook dinner though and am not sure how much evening time I will get, so serving up now in case you get to take a gander. Lots and lots of updates - addressed all the feedback and made other changes... DaemonsWe have an interesting potential problem in that Twilio, etc. actually retries on its own for awhile, so unless they can truly say they failed re-sending on our side we will end up sending duplicates. Further, Twilio, etc. claim to fail rarely and that most errors are configuration or rate limiting errors, so retrying naively in the doWork method is going to not work or spam SMS to folks. As such, re-jiggered this a bit to have a poll inside the "doWork" method, and before checkin I think I need to retry if and only if the external provider has said "we definitely failed." thoughts? any pro-tips for debugging daemons and workers? finding it hard to surface phlog, echo, etc and will probably write to a temp file if there's no better way to surface info. in doWork, if everything went okay, you just return? what if things went badly but not "fail permanently" badly? is there a way to sleep for a bit then hop in the top of the doWork method without that counting as a try? (answers to these questions would let me implement the best retry strategy possible as cleanly as possible.) Comment Actions
Ideally, we should distinguish between "this is failed forever" (e.g., bad "to" number) and "this is appropriate to retry" (e.g. rate limit). Thinking about this, for v0, I kind of thing we should maybe just never retry. A lot of mail we really want to try hard to deliver, but the only SMSes we're going to send right now are multi-factor SMSes, which are no good if they take more than a minute or so to show up. Generally, SMSes are likely to be time sensitive, and delivering them much later is probably annoying at best (your phone blows up) and confusing at worst (you get a bunch of SMS codes which aren't valid anymore, or alerts for things that already happened). So the logic would look like:
Then based on our experience in the real world we can see if that's good enough or not? My guess is that it will be like 98% good, and then adding retries for specific errors (e.g., rate limit) will get us to like 99%+ good.
Stop all the daemons, then run bin/phd debug taskmaster to guarantee that process gets to process everything. Then all the log(), etc., will show up in your console.
You can throw new PhabricatorWorkerYieldException(15); to mean (approximately): don't increase the failure count, and retry this task in about 15-ish seconds. (It may take longer than 15 seconds before the task gets retried, but that should be OK here I think).
Comment Actions Also, we use "multiplex" elsewhere but I think your usage is actually correct and they should all be "demultiplex". I think I just never looked it up, I'll maybe fix that at some point.
Comment Actions Yeah, this seems quite reasonable to me. I think it's perfectly fine to leave 'em in "probably sent, not totally sure" for now.
|