[asterisk-dev] [Code Review] Add voicemail message IDs where needed on startup; Add message IDs to ODBC voicemail

[Matt Jordan]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1898/#review6214
-----------------------------------------------------------



/team/mmichelson/trunk-digiumphones/apps/app_voicemail.c
<https://reviewboard.asterisk.org/r/1898/#comment11452>

    If it hasn't been done already, the schema files need to be updated with the new column



/team/mmichelson/trunk-digiumphones/apps/app_voicemail.c
<https://reviewboard.asterisk.org/r/1898/#comment11450>

    Blob



/team/mmichelson/trunk-digiumphones/apps/app_voicemail.c
<https://reviewboard.asterisk.org/r/1898/#comment11451>

    This is an incredibly expensive operation, as it has to load each voicemail meta file that a user has into memory.  I have no idea how expensive this is going to get in the 'real world', but I imagine that if someone hasn't cleaned up their voicemail folders (or has a 100 old messages), its going to take a long time.  I have seen some issues reported where response times from some IMAP servers are on the order of seconds - having to pull back each individual message could make this operation run for minutes.
    
    Additionally, once all voicemails have been populated with message identifiers, this step is no longer necessary, but we would continue to perform it.  Doing it once during an upgrade is one thing, doing it repeatedly as part of the normal operation is pretty harsh.
    
    I can imagine a few ways (none of which are all that palatable) to alleviate this:
    1. Have a configuration override to disable it.  After a week or so of painfully slow voicemail user loads, a sys admin can at least disable it.
    2. Perform the action in module load, after all voicemail users are loaded.  This would be a one time action, after which it would not impact normal operations.  This could be combined with option #1 to make it so that it only occurs a single time.
    3. Perform the action lazily by not doing it in find_or_create.  Instead, do it when we load a voicemail into memory - if a unique ID is missing, generate it and re-store the meta file.
    


- Matt


On May 2, 2012, 4:04 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1898/
> -----------------------------------------------------------
> 
> (Updated May 2, 2012, 4:04 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> There are two main things added here.
> 
> 1. Voicemail APIs added in trunk rely on voicemail messages possessing a "msg_id" in their metadata. This is how messages are looked up when they are to be moved, played, removed, and forwarded. The issue is that msg_id is a new piece of data and is not present on voicemails saved in older versions of Asterisk. This patch aims to fix that. When voicemail is loaded, each user's voicemail box is searched and each message is checked for msg_id. If it is not present, one is generated and added to that message's metadata. For ODBC, I added a method that specifically is used to issue a SQL UPDATE command to set the msg_id for the message. IMAP does not have this ability because I have no idea how to alter/update stored voicemails in IMAP.
> 
> 2. I noticed that the ODBC function for storing voicemails did not bother to store the msg_id. I have updated the code to do this properly.
> 
> 
> Diffs
> -----
> 
>   /team/mmichelson/trunk-digiumphones/apps/app_voicemail.c 364886 
> 
> Diff: https://reviewboard.asterisk.org/r/1898/diff
> 
> 
> Testing
> -------
> 
> For 1, I have manually tested by leaving a voicemail using Asterisk 1.8 and then loading app_voicemail.so in trunk-digiumphones. The metadata file for the message had a msg_id added to it as expected.
> 
> For 2, I attempted to test leaving messages via ODBC, but I ran into failures to execute the SQL INSERT statement when storing the voicemail. I attempted on a clean trunk checkout and on a clean 1.8 checkout and got the same error, so I figure it's not necessarily a coding issue but a setup issue on my end. I figured that's not enough to stop me from posting the review now and getting my setup fixed up after. If it turns out I find any glaring bugs in the code, I'll update the diff here.
> 
> 
> Thanks,
> 
> Mark
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120514/285553d3/attachment-0001.htm>