[asterisk-dev] [Code Review] Trunk changes for queue ignorebusy. Allow setting of ignorebusy on a queue member through configs, CLI, and AMI.

Tue Apr 17 14:48:23 CDT 2012

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

I strongly suggest that we use a different name for this option than "ignorebusy". The name does a poor job of explaining what the option actually does. When I first saw it, I thought it meant that when ringing queue members, the queue should ignore those that are currently busy. However, that's not what it means at all. Rather, it means for the queue to ignore the fact that the member is busy and ring him anyway. Even with that understanding, the option name is still misleading because the option does not actually apply to busy members (i.e. members whose device state is "busy") but to in use members (i.e. members whose device state is "in use").

My suggestion for an amended name is going to rock your world. Ready for it? ... ... ... "ringinuse". This name makes it more clear that it corresponds to the queue-level "ringinuse" option and has the same connotation as the queue option. The name "ignorebusy" can be kept for backwards compatibility for realtime queue members, but all documentation and the CLI command to set the option for a queue member needs to use the new name.

In addition to the name change, I'm also proposing a behavior change for how the option works. Currently, if ringinuse for a queue is set to "no" then the ignorebusy setting for a queue member has no effect. The only way ignorebusy has any effect is if ringinuse is set to yes. My feeling on the matter is that ringinuse on a queue is something that should be inherited by the by each member initially, but it can be overridden if the ignorebusy setting on the member is different.

So for instance, if the queue is set to ringinuse = no, but there are some members that have ignorebusy set, then those members should be ringable even though the queue's setting is to not ring members who are in use.

/trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/1861/#comment11018>

    Please rename this function to something else. Something like "set_member_ignorebusy_helper" would be fine.

/trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/1861/#comment11019>

    Setting rtignorebusy this way is overkill. Since the ignorebusy int is a boolean, you can easily use ternary logic to get the desired string instead of using a printf-like function.

    char *rtignorebusy = ignorebusy ? "on" : "off";

/trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/1861/#comment11020>

    Using an increment here is odd since this isn't being used to cumulatively tally some number. It's a boolean, so setting it to 1 makes it more clear what it's used for (plus it might be about a microsecond faster too).

/trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/1861/#comment11021>

    This is another place where incrementing doesn't really make any sense given that the values in question are booleans.

/trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/1861/#comment11012>

    Persistent members need to have the same ignorebusy value they had when they had previously when app_queue was running. Setting ignorebusy to 1 when persistent queue members are reloaded is not good.

/trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/1861/#comment11013>

    AddQueueMember dialplan application should allow setting ignorebusy just the same as any other member option.

/trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/1861/#comment11014>

    Another instance where ignorebusy should be settable when adding the queue member.

/trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/1861/#comment11022>

    Why are you doing these casts here? set_member_ignorebusy takes const char * for both queuename and interface, so this cast is bizarre.

/trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/1861/#comment11017>

    Another place where ignorebusy should be settable in the command to add a queue member.

/trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/1861/#comment11016>

    The comment is a bit misleading since the '2' can be either the literal "penalty" or "ignorebusy". It has no bearing on the way this function works so it's not a huge deal, but comment accuracy is a good thing.

/trunk/configs/queues.conf.sample
<https://reviewboard.asterisk.org/r/1861/#comment11015>

    This does not adequately explain what the ignorebusy field for a queue member actually *is*.

- Mark

On April 9, 2012, 12:59 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1861/
> -----------------------------------------------------------
> 
> (Updated April 9, 2012, 12:59 p.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson, irroot, and Matt Jordan.
> 
> 
> Summary
> -------
> 
> This patch adds methods to set the queue member value ignorebusy through AMI commands, CLI, and while creating a member via configs.  They are all based on the equivalent methods for setting queue penalties.
> 
> 
> This addresses bug ASTERISK-19536.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19536
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_queue.c 361654 
>   /trunk/configs/queues.conf.sample 361654 
> 
> Diff: https://reviewboard.asterisk.org/r/1861/diff
> 
> 
> Testing
> -------
> 
> Mostly just using each of the different values in a variety of ways to set the ignorebusy value and then checking whether the flag was raised or not with the CLI show queue command (which I added some display for.  It displays (not ignorebusy) when the flag is lowered since ignorebusy is the normal state... in spite of the double negative terminology which makes me cringe.
> 
> 
> Thanks,
> 
> jrose
> 
>

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