[Asterisk-code-review] taskprocessor: Enable subsystems and overload by subsystem (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/11001 )

Change subject: taskprocessor:  Enable subsystems and overload by subsystem
......................................................................


Patch Set 3: Code-Review-1

(12 comments)

https://gerrit.asterisk.org/#/c/11001/3/configs/samples/pjsip.conf.sample
File configs/samples/pjsip.conf.sample:

https://gerrit.asterisk.org/#/c/11001/3/configs/samples/pjsip.conf.sample@1119
PS3, Line 1119:                 ; "none": No overload detection will be performed.
Need to note that this value is risky to use as not backing off on new work puts your system at risk of failing.


https://gerrit.asterisk.org/#/c/11001/3/configs/samples/pjsip.conf.sample@1121
PS3, Line 1121:                 ; "pjsip_only": Only pjsip taskprocessor overloads will trigger.
Need to note that this is a compromise.  As we will decline new work because of taskprocessor overload from taskprocessors directly associated with pjsip and not any submodules that pjsip may be feeding.


https://gerrit.asterisk.org/#/c/11001/3/contrib/ast-db-manage/config/versions/f3c0b8695b66_taskprocessor_overload_trigger.py
File contrib/ast-db-manage/config/versions/f3c0b8695b66_taskprocessor_overload_trigger.py:

https://gerrit.asterisk.org/#/c/11001/3/contrib/ast-db-manage/config/versions/f3c0b8695b66_taskprocessor_overload_trigger.py@27
PS3, Line 27:     
blob


https://gerrit.asterisk.org/#/c/11001/3/main/stasis.c
File main/stasis.c:

https://gerrit.asterisk.org/#/c/11001/3/main/stasis.c@682
PS3, Line 682: 		ast_taskprocessor_build_name(tps_name, sizeof(tps_name), "sub%c/%s",
Suggest renaming these stasis subscription taskprocessors to:
stasis/%c:%s


https://gerrit.asterisk.org/#/c/11001/3/main/taskprocessor.c
File main/taskprocessor.c:

https://gerrit.asterisk.org/#/c/11001/3/main/taskprocessor.c@94
PS3, Line 94: Anyything
sp


https://gerrit.asterisk.org/#/c/11001/3/main/taskprocessor.c@594
PS3, Line 594: 	AST_VECTOR_APPEND(&overloaded_subsystems, s);
AST_VECTOR_APPEND can fail and leak s.

Suggest renaming s to sub to not use single letter variables.


https://gerrit.asterisk.org/#/c/11001/3/main/taskprocessor.c@624
PS3, Line 624: static void tps_alert_add(struct ast_taskprocessor *tps, int delta)
The changes to this function are not good.

You need to use delta to see if pjsip went into or out of alert.  tps_alert_count is the global count of taskprocessors in alert.

You could conceivably have more than one taskprocessor in alert status.  With the new subsystem grouping you need to keep track of how many taskprocessors in a subsystem are in alert too.


https://gerrit.asterisk.org/#/c/11001/3/res/res_pjsip.c
File res/res_pjsip.c:

https://gerrit.asterisk.org/#/c/11001/3/res/res_pjsip.c@1847
PS3, Line 1847: 				<configOption name="taskprocessor_overload_trigger" default="global">
Specifying a default here is useless.  Nothing uses the default value specified here.


https://gerrit.asterisk.org/#/c/11001/3/res/res_pjsip.c@1856
PS3, Line 1856: 							<enum name="none"><para>No overload detection will be performed.</para></enum>
              : 							<enum name="global"><para>(default) Any taskprocessor overload will trigger.</para></enum>
              : 							<enum name="pjsip_only"><para>Only pjsip taskprocessor overloads will trigger.</para></enum>
Need to add the same warnings here about "none" and "pjsip_only" as in the sample file.


https://gerrit.asterisk.org/#/c/11001/3/res/res_pjsip.c@5011
PS3, Line 5011: 	sip_threadpool = ast_threadpool_create("SIP", NULL, &options);
Change the passed name string from "SIP" to "pjsip" and fix ast_threadpool_create() to create taskprocessors with these names:

<name>/pool
<name>/pool-control

The created taskprocessors are directly used by the pjsip "subsystem".

Really should do this regardless of the whether the patch ever gets committed just to better show what the taskprocessors actually are.


https://gerrit.asterisk.org/#/c/11001/3/res/res_pjsip/config_global.c
File res/res_pjsip/config_global.c:

https://gerrit.asterisk.org/#/c/11001/3/res/res_pjsip/config_global.c@705
PS3, Line 705: 			"global", overload_trigger_handler, overload_trigger_to_str, NULL, 0, 0);
one too many indention tabs


https://gerrit.asterisk.org/#/c/11001/3/res/res_pjsip/config_global.c@705
PS3, Line 705: "global"
Use
overload_trigger_map[DEFAULT_TASKPROCESSOR_OVERLOAD_TRIGGER]



-- 
To view, visit https://gerrit.asterisk.org/11001
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I8c19068bb2fc26610a9f0b8624bdf577a04fcd56
Gerrit-Change-Number: 11001
Gerrit-PatchSet: 3
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Friendly Automation (1000185)
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Fri, 15 Feb 2019 23:48:29 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190215/536e8ebd/attachment-0001.html>