Summary:	ASTERISK-11350: [patch] Optimized update_realtime_member_field function
Reporter:	Atis Lezdins (atis)	Labels:
Date Opened:	2008-01-31 17:21:30.000-0600	Date Closed:	2008-05-21 13:25:55
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Applications/app_queue
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) realtime_uniqueid_v4.patch ( 1) realtime_uniqueid_v5.patch ( 2) realtime_uniqueid_v6.patch ( 3) rt_uniqueid_1.4.patch ( 4) rt_uniqueid_2.patch ( 5) rt_uniqueid_3.patch ( 6) rt_uniqueid.patch
Description:	Currently update_realtime_member_field does first load to find out current uniqueid of member by interface and queue name, and only then updates corresponding member record by uniqueid. This can be optimized by storing uniqueid in member structure, making update_realtime_member_field much more simpler and eliminating extra access to realtime engine. It won't break current systems (without uniqueid), as they are not updating anyway.
Comments:	By: Atis Lezdins (atis) 2008-01-31 17:55:06.000-0600 in last patch i replaced  if(str) with  if (!ast_strlen_zero(str)) within rt_handle_member_record() By: jmls (jmls) 2008-02-01 01:44:43.000-0600 please make the uniqueid a non-integer (i.e. a string) I have a GUID that uniquely identifies a record within a table (queue etc) and do not want to have to add a unique integer id to my tables. By: Atis Lezdins (atis) 2008-02-01 04:54:02.000-0600 ok, in third patch rt_uniqueid is stored as char[80] By: Atis Lezdins (atis) 2008-02-01 05:09:39.000-0600 Backport for 1.4 - probably won't make to it because it's not a bugfix, but for those interested. Will conflict with backport of state_interface. By: Mark Michelson (mmichelson) 2008-02-01 10:12:23.000-0600 I've been looking over this patch, and it definitely is a good optimization. I tried thinking of times when this could cause a problem. I was thinking of circumstances like the following: 1. Load member information from realtime 2. Member is deleted from the backend. 3. Before we read info from realtime again, we attempt to update a field based on what we have in memory. In the above scenario, the update will fail, but that's no problem, since the operation will still succeed for what we have in memory and that member will be marked for deletion after the next read from realtime anyway. What is more problematic is the following scenario: 1. Load member information from realtime 2. Member is deleted from realtime and then re-added (therefore having a new uniqueid). 3. Before we read info from realtime again, we attempt to update a field based on what we have in memory. In this scenario, the update will again fail since no member with the uniqueid we have in memory will exist. In this case, I could foresee issues. For instance, someone could attempt to pause a member and even though it would pause the member in memory, the paused status would not be reflected in realtime due to the failed update. Then, the next time member information is read from realtime, the paused status of the member could be changed to an undesired value. Of course, if a member was removed and then re-added to the database, should the user have any expectation of state to carry from the previously defined member to the new one? It's an arguable point and I'd like to hear your opinions on the matter. By: Atis Lezdins (atis) 2008-02-01 10:49:59.000-0600 So, the first case is ok for you? About second case - i have already thought of it, but my opinion is that UNIQUEID in database should actually identify the member, not interface or membername. It's just a matter of DB design. If you still wish to delete from db, update paused, re-add in db and have it paused after that, i think it could be done by updating "rt_uniqueid" in rt_handle_member_record if member is already there (looked up by interface in local list), but then again - you would have it in status that was set in db when re-adding. So, fail of update_realtime_member_field should return error, and don't update paused, unless it's successfully updated in db. By: Mark Michelson (mmichelson) 2008-02-07 10:15:06.000-0600 atis and I talked on IRC. Just so there's a record of it, I just agreed with what he was saying in his latest note, and he's going to upload a patch when he has the situation reworked. By: jmls (jmls) 2008-05-03 14:27:00 atis, did you manage to update the patch as suggested by putnopvut ? By: Atis Lezdins (atis) 2008-05-07 16:52:09 sorry, no progress so far on this. however due to this punch i will try to do this in next week. additionally the topic on realtime queue status is currently under discussion, so as this is small part of it, i feel like finishing this :) By: Atis Lezdins (atis) 2008-05-20 03:07:23 Ok, the latest patch realtime_uniqueid_v5.patch should cover every case. If realtime member is paused, and realtime operation fails - it will give a warning, and pause status won't be updated. There's also optimization in pause loop - if member is found, and pausing is not done in all queues - stop iterating queues. while working on this, i realized that rt_handle_member should also be modified to look up member by uniqueid not interface, as having uniqueid in realtime should allow to change interface while leaving the same id. By: Atis Lezdins (atis) 2008-05-20 04:57:45 Huh, my mistake. Fixed locking problem within set_member_paused. By: Mark Michelson (mmichelson) 2008-05-21 11:13:54 I gave v6 a look, and it appears to be good. The only nitpick I have is a coding guidelines one. Typically, if it can be done, we like to not indent large blocks if they don't need to be indented. in set_member_paused, there is a section, if (failed), where the if portion contains a single statement and the else portion contains a large block. If the if portion were modified to be if (failed) {    /* log message omitted */    ao2_ref(mem, -1);    continue; } then the else part would not need to be inside an else block at all, and we could lose a level of indention as a result. Since this is such a minor detail and you have already done a lot of work to get this patch to where it is, I will make this small change myself when I commit it. Thanks for the contribution! By: Digium Subversion (svnbot) 2008-05-21 13:25:05 Repository: asterisk Revision: 117517 U   trunk/apps/app_queue.c ------------------------------------------------------------------------ r117517 | mmichelson | 2008-05-21 13:24:54 -0500 (Wed, 21 May 2008) | 14 lines Optimize the update_realtime_member_field function by not having to query the database for the member and instead using a cached uniqueid. Special thanks to atis for creating this and for keeping it up to date with necessary changes (closes issue ASTERISK-11350) Reported by: atis Patches:      realtime_uniqueid_v6.patch uploaded by atis (license 242) Tested by: atis ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=117517 By: Digium Subversion (svnbot) 2008-05-21 13:25:55 Repository: asterisk Revision: 117518 _U  branches/1.6.0/ ------------------------------------------------------------------------ r117518 | mmichelson | 2008-05-21 13:25:54 -0500 (Wed, 21 May 2008) | 21 lines Blocked revisions 117517 via svnmerge ........ r117517 | mmichelson | 2008-05-21 13:31:05 -0500 (Wed, 21 May 2008) | 14 lines Optimize the update_realtime_member_field function by not having to query the database for the member and instead using a cached uniqueid. Special thanks to atis for creating this and for keeping it up to date with necessary changes (closes issue ASTERISK-11350) Reported by: atis Patches:      realtime_uniqueid_v6.patch uploaded by atis (license 242) Tested by: atis ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=117518