[asterisk-dev] [Code Review] 3607: [app_queue] Add the optional ability to load queue rules from realtime.

[Mark Michelson]

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


Thanks for the patch. Most of the recommendations I have are small coding guidelines-related quibbles.


trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/3607/#comment22218>

    Coding guidelines: Place a space after the if.



trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/3607/#comment22219>

    I recommend extracting everything in this if block into its own function. It will help to keep the already large reload_queue_rules function from doubling in size.



trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/3607/#comment22220>

    Coding guidelines: Add a space after the if.



trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/3607/#comment22221>

    Coding guidelines: This line is exceedingly long. I suggest separating the declarations of these variables onto separate lines.



trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/3607/#comment22222>

    Coding guidelines: Place a space after the comma.



trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/3607/#comment22223>

    Allocation failure messages aren't necessary since ast_calloc() already will print out an error.



trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/3607/#comment22225>

    atoi() does not detect errors (such as the input not being an integer) and will just return 0 on bad values. You can change this to:
    
    if (!timestr || sscanf(timestr, "%30d", &penaltychangetime) != 1)
    
    This will perform a more type-safe method of getting the value. The %30 in the format specifier is there as (an admittedly arbitrary) a way of preventing input of overly large numbers.



trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/3607/#comment22226>

    While log messages do not need to be super formal, avoid overly informal or slangy words like "gonna". Also, I would suggest printing timestr in this message so that it is more obvious to the user what failed to parse.



trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/3607/#comment22224>

    Another case where the error messages isn't needed.



trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/3607/#comment22227>

    Coding Guidelines: This line is exceptionally long and should be broken up into multiple lines.



trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/3607/#comment22229>

    Use sscanf instead of atoi.



trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/3607/#comment22228>

    Coding Guidelines: This line should be broken into two lines.



trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/3607/#comment22230>

    Use sscanf instead of atoi.



trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/3607/#comment22231>

    Coding Guidelines: Add a space after the if.



trunk/apps/app_queue.c
<https://reviewboard.asterisk.org/r/3607/#comment22232>

    This line can be deleted.


- Mark Michelson


On June 11, 2014, 10:04 a.m., Michael K. wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3607/
> -----------------------------------------------------------
> 
> (Updated June 11, 2014, 10:04 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23823
>     https://issues.asterisk.org/jira/browse/ASTERISK-23823
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch gives the ability (optional) to keep queuerules in realtime (important to notice that rules would be loaded from both realtime and file too)
> To use add to queuerules.conf general section with option to turn the relatime on (note that after patch there is no option to call rule "general" as it is reserved for queuerules settings):
> 
> [general]
> realtime_rules = yes
> 
> and add the realtime setting to extconfig.conf, for example in my case it's mysql and it looks like(will attach the .sql with create for table):
> 
> queue_rules => mysql,asterisk,queue_rules
> 
> in table as you can see you need to specify rule name and as in regular rules you can use relative setings for min and max (with "-" and "+", e.g. "+100")
> if one of the rule parameters is wrong it would be ignored, basically validation is like from file.
> here is the the example of table context as an example:
> 
> rule_name, time, min_penalty, max_penalty
> 'default', '10', '20', '30'
> 'test2', '20', '30', '55'
> 'test2', '25', '-11', '+1111'
> 'test2', '400', '112', '333'
> 'test3', '0', '4564', '46546'
> 'test_rule', '40', '15', '50'
> 
> which would result in :
> 
> Rule: default
> 	After 10 seconds, adjust QUEUE_MAX_PENALTY to 30 and adjust QUEUE_MIN_PENALTY to 20
> Rule: test2
> 	After 20 seconds, adjust QUEUE_MAX_PENALTY to 55 and adjust QUEUE_MIN_PENALTY to 30
> 	After 25 seconds, adjust QUEUE_MAX_PENALTY by 1111 and adjust QUEUE_MIN_PENALTY by -11
> 	After 400 seconds, adjust QUEUE_MAX_PENALTY to 333 and adjust QUEUE_MIN_PENALTY to 112
> Rule: test3
> 	After 0 seconds, adjust QUEUE_MAX_PENALTY to 46546 and adjust QUEUE_MIN_PENALTY to 4564
> Rule: test_rule
> 	After 40 seconds, adjust QUEUE_MAX_PENALTY to 50 and adjust QUEUE_MIN_PENALTY to 15
> 
> 
> If you use realtime, the rules will be always reloaded (no cache option here), in other words it would delete all rules and re-add them from realtime and file. It's important to understand that with realtime on it would reload rules from file doesn't matter if file was or was not change.
> While with the option off it would act exactly like it was before patch (file would not be reloaded if it was not changed)
> 
> 
> Diffs
> -----
> 
>   trunk/apps/app_queue.c 415442 
> 
> Diff: https://reviewboard.asterisk.org/r/3607/diff/
> 
> 
> Testing
> -------
> 
> Currently the tests were made on development machine.
> 
> 
> Thanks,
> 
> Michael K.
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140617/39f8d447/attachment-0001.html>