<p>Patch set 3:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/11001">View Change</a></p><p>12 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11001/3/configs/samples/pjsip.conf.sample">File configs/samples/pjsip.conf.sample:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11001/3/configs/samples/pjsip.conf.sample@1119">Patch Set #3, Line 1119:</a> <code style="font-family:monospace,monospace"> ; "none": No overload detection will be performed.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Need to note that this value is risky to use as not backing off on new work puts your system at risk of failing.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11001/3/configs/samples/pjsip.conf.sample@1121">Patch Set #3, Line 1121:</a> <code style="font-family:monospace,monospace"> ; "pjsip_only": Only pjsip taskprocessor overloads will trigger.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="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:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11001/3/contrib/ast-db-manage/config/versions/f3c0b8695b66_taskprocessor_overload_trigger.py@27">Patch Set #3, Line 27:</a> <code style="font-family:monospace,monospace"> </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">blob</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11001/3/main/stasis.c">File main/stasis.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11001/3/main/stasis.c@682">Patch Set #3, Line 682:</a> <code style="font-family:monospace,monospace"> ast_taskprocessor_build_name(tps_name, sizeof(tps_name), "sub%c/%s",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Suggest renaming these stasis subscription taskprocessors to:<br>stasis/%c:%s</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11001/3/main/taskprocessor.c">File main/taskprocessor.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11001/3/main/taskprocessor.c@94">Patch Set #3, Line 94:</a> <code style="font-family:monospace,monospace">Anyything</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">sp</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11001/3/main/taskprocessor.c@594">Patch Set #3, Line 594:</a> <code style="font-family:monospace,monospace"> AST_VECTOR_APPEND(&overloaded_subsystems, s);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">AST_VECTOR_APPEND can fail and leak s.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Suggest renaming s to sub to not use single letter variables.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11001/3/main/taskprocessor.c@624">Patch Set #3, Line 624:</a> <code style="font-family:monospace,monospace">static void tps_alert_add(struct ast_taskprocessor *tps, int delta)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The changes to this function are not good.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11001/3/res/res_pjsip.c">File res/res_pjsip.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11001/3/res/res_pjsip.c@1847">Patch Set #3, Line 1847:</a> <code style="font-family:monospace,monospace"> <configOption name="taskprocessor_overload_trigger" default="global"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Specifying a default here is useless. Nothing uses the default value specified here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11001/3/res/res_pjsip.c@1856">Patch Set #3, Line 1856:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> <enum name="none"><para>No overload detection will be performed.</para></enum><br> <enum name="global"><para>(default) Any taskprocessor overload will trigger.</para></enum><br> <enum name="pjsip_only"><para>Only pjsip taskprocessor overloads will trigger.</para></enum><br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Need to add the same warnings here about "none" and "pjsip_only" as in the sample file.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11001/3/res/res_pjsip.c@5011">Patch Set #3, Line 5011:</a> <code style="font-family:monospace,monospace"> sip_threadpool = ast_threadpool_create("SIP", NULL, &options);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Change the passed name string from "SIP" to "pjsip" and fix ast_threadpool_create() to create taskprocessors with these names:</p><p style="white-space: pre-wrap; word-wrap: break-word;"><name>/pool<br><name>/pool-control</p><p style="white-space: pre-wrap; word-wrap: break-word;">The created taskprocessors are directly used by the pjsip "subsystem".</p><p style="white-space: pre-wrap; word-wrap: break-word;">Really should do this regardless of the whether the patch ever gets committed just to better show what the taskprocessors actually are.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11001/3/res/res_pjsip/config_global.c">File res/res_pjsip/config_global.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11001/3/res/res_pjsip/config_global.c@705">Patch Set #3, Line 705:</a> <code style="font-family:monospace,monospace"> "global", overload_trigger_handler, overload_trigger_to_str, NULL, 0, 0);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">one too many indention tabs</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11001/3/res/res_pjsip/config_global.c@705">Patch Set #3, Line 705:</a> <code style="font-family:monospace,monospace">"global"</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Use<br>overload_trigger_map[DEFAULT_TASKPROCESSOR_OVERLOAD_TRIGGER]</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/11001">change 11001</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/11001"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I8c19068bb2fc26610a9f0b8624bdf577a04fcd56 </div>
<div style="display:none"> Gerrit-Change-Number: 11001 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation (1000185) </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 15 Feb 2019 23:48:29 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>