[Home]

Summary:ASTERISK-01796: [patch] Priority Queues
Reporter:mman (mman)Labels:
Date Opened:2004-06-10 11:37:45Date Closed:2008-01-15 15:00:22.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Applications/app_queue
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) prioqueue.patch
( 1) prioqueue1.patch
Description:This patch modifies the behavior of the current Queue
application. It provides the ability to operate them as
priority queues in addition to the current FIFO mode. This
gives the ability to queue a call not at the end of the
queue but anywhere in the queue, according to the call's
priority.
Now you can have just one queue servicing all the calls
(more important and less important) with the right order.


****** ADDITIONAL INFORMATION ******

The priority of a call entering a queue is determined by a
special channel variable, QUEUE_PRIO. Higher values of this
variable mean higher priority. By not setting this variable,
all calls have the same priority, 0, by default (FIFO). E.g.

; Important clients
exten => 111,1,Playback(welcome)
exten => 111,2,SetVar(QUEUE_PRIO=10)
exten => 111,3,Queue(support)

; Less important clients
exten => 112,1,Playback(welcome)
exten => 112,2,SetVar(QUEUE_PRIO=5)
exten => 112,3,Queue(support)
Comments:By: mman (mman) 2004-06-10 11:40:58

Ooops, I didn't set the general description of this bug.
This should be "[patch] Priority queues".
Sorry!
By: Malcolm Davenport (mdavenport) 2004-06-10 12:09:59

Thanks :)
By: Mark Spencer (markster) 2004-06-12 11:40:22

Do we already have a disclaimer on file for you, mic?  If so, in the future you might put it in the bug decription, thanks!

On the one hand it would have been nice to have the priority on the Queue() command line, but there's already so much stuff that I'm not sure it's any easier than the variable.  What are your feelings?
By: mman (mman) 2004-06-15 04:44:11

Mark, I had the same thoughts about where to place the priority, and I believe
that using the chan var is the cleanest way. Let's not put another option on the
Queue arguments. Anyway, if this causes problems I would be happy to add it as
just another option.

A disclaimer has not been sent, but I'll fax it today or tomorrow (I'll let you
know).

Michael.
By: mman (mman) 2004-06-17 11:12:29

A disclaimer for that has been sent.

Michael.
By: Mark Spencer (markster) 2004-06-20 03:23:27

Great, got the disclaimer.  Would like to discuss a few small things:

1) Avoid superfluous whitespace changes.  I use tab length = 4 in my own editing (and use tabs instead of spaces for indenting).

2) To simplify the logic for insert and reduce duplication, I suggest as follows:

while(cur || prev) {
 if (!cur || (qe->prio <= cur->prio)) {
      /* add to list */
      if (!cur)
          break;
 }
 cur->pos = ++pos;
 prev = cur;
 cur = cur->next;
}

3) Can you explain the "callattempt" logic?

4) Would sscanf be easier than strtol?

if (sscanf(s, "%d", &foo) == 1) /* valid */
By: mman (mman) 2004-06-21 06:03:13

Mark,

1) There are just some whitespace changes just to make code aligned and
eliminate extra blank lines. I also use the same editing params.

2) I don't get it. How the new way is simpler than the old one?

3) I had some problems where multiple channels were trying to ring a queue member
when none was there. There were cases, when using priorities, in a situation
like this, where a higher priority channel would talk to a queue member after
a lower priority channel, although they were waiting in the queue in reverse order. The 'callattempt' flag indicates whether a channel is trying to ring a queue member, so other channels will wait (it won't take much, anyway) for it
to finish. It actually helps the queue to maintain the order of the serviced
channels.

4) I prefer strtol but we could change it to sscanf.
By: Mark Spencer (markster) 2004-06-21 11:14:45

1) It's not killer, but clearly the first several lines of the patch are all just whitespace changes.  If it's not easy to change don't worry about it.

2) My version causes the insertion code to exist in only a single place.  You could also make it a function call, but I don't like that we have two copies of the same code in the same place.

3) As I am reading it, callattempt is set, cleared, and checked exclusively with the parent mutex locked.  In other words, there is no situation in which callattempt is set or checked without the qe->parent->lock being held, right?

4) How do you determine with strtol that QUEUE_PRIO is not a valid value (e.g. is 'foo' instead of '5')?  It looks like you could use "endptr" to do it, but sscanf might make that an easier check.  If you prefer strtol still you may certainly use it.  Just an observation.
By: mman (mman) 2004-06-21 12:03:06

1) OK.
2) Agree on that too.
3) I'll look at it and see if it's really needed. Probably an intermediate patch
fixed the problem I had.
4) Both are fine with me.

So, I can send a revised patch, possibly tomorrow, but you are also welcome
to edit and throw it in CVS.
By: mman (mman) 2004-06-22 11:58:45

Mark, your method (2) doesn't eliminate the duplicate insertion code. Consider
the case where you must add something into an empty queue (to cover this
one you need the insertion code outside the while loop as well).
By: Mark Spencer (markster) 2004-06-22 12:52:22

Then lets do it as a (inline) function?
By: mman (mman) 2004-06-23 06:18:28

Patch updated (inline function for entry insertion, usage of sscanf, removed unused callattempt flag)
By: jrollyson (jrollyson) 2004-06-23 14:56:10

Ok to zap the first patch?
By: Mark Spencer (markster) 2004-06-23 16:19:33

Added to CVS, thanks!
By: Digium Subversion (svnbot) 2008-01-15 15:00:22.000-0600

Repository: asterisk
Revision: 3288

U   trunk/apps/app_queue.c

------------------------------------------------------------------------
r3288 | markster | 2008-01-15 15:00:22 -0600 (Tue, 15 Jan 2008) | 2 lines

Merge in-access updates for queue priorities (bug ASTERISK-1796)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=3288