<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/1898/">https://reviewboard.asterisk.org/r/1898/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 14th, 2012, 2:42 p.m., <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/1898/diff/1/?file=27638#file27638line11425" style="color: black; font-weight: bold; text-decoration: underline;">/team/mmichelson/trunk-digiumphones/apps/app_voicemail.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static struct ast_vm_user *find_or_create(const char *context, const char *box)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">11425</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">add_message_ids</span><span class="p">(</span><span class="n">vmu</span><span class="p">))</span> <span class="p">{</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">11426</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ast_log</span><span class="p">(</span><span class="n">LOG_WARNING</span><span class="p">,</span> <span class="s">"Unable to add unique message IDs for all messages for voicemail user %s@%s</span><span class="se">\n</span><span class="s">"</span><span class="p">,</span> <span class="n">vmu</span><span class="o">-></span><span class="n">mailbox</span><span class="p">,</span> <span class="n">vmu</span><span class="o">-></span><span class="n">context</span><span class="p">);</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">11427</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">}</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.
</pre>
</blockquote>
<p>On May 18th, 2012, 8:30 a.m., <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Yep, this operation is pretty painful. All three of your options have some drawbacks, but I agree that something different should probably be done.
1. This would work, but the problem is that it would be hard to enforce usage of it. We'd have to instruct people to run Asterisk 11 with the option disabled first, then go ahead and enable it for all future runs. This is too complicated.
2. This is actually what I had been aiming for, but you're right that on reloads of voicemail configuration, the operation will run as well. Making it run once per voicemail startup would be a possible option.
3. This is probably my favorite of the options just because it means only having to update metadata when needed. Since we're retrieving metadata anyway, the operation of checking for a message ID will be dwarfed by the complexity of other processing being done. The only time I can see performance being a problem is the first time a mailbox snapshot is retrieved for a mailbox.
I believe I'm going with option 3 here. I will modify the functions called by RETRIEVE macros to check for the existence of a message ID. If none exists, then one will be created and the metadata will be updated appropriately.
Note that I still have no idea about how to update an IMAP message's metadata, so I'm going to start with file storage and ODBC for now.</pre>
</blockquote>
<p>On May 18th, 2012, 8:47 a.m., <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Metadata for an IMAP message is stored in custom headers, where the header format typically follows:
"X-Asterisk-VM-[foo]"
imap_retrieve_file will have to be updated to look for whatever header we decide to add for a unique ID and write that header out to the meta data file, while make_email_file will have to add the unique message ID header to the IMAP message and store the unique ID in it.
Obviously, if imap_retrieve_file fails to pull the unique ID header that we create, it should come up with a suitable replacement (similar to your approach for file/ODBC backends)</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I understand how the headers work for IMAP voicemails. What I'm unsure of is a function in the IMAP library we use that will allow for us to add new metadata onto an existing message. A really hacky way to do it would be to retrieve the message, add the message ID, delete the old message, and store the new one. I would rather avoid this if possible though.</pre>
<br />
<p>- Mark</p>
<br />
<p>On May 2nd, 2012, 4:04 p.m., Mark Michelson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Mark Michelson.</div>
<p style="color: grey;"><i>Updated May 2, 2012, 4:04 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/team/mmichelson/trunk-digiumphones/apps/app_voicemail.c <span style="color: grey">(364886)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1898/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>