<p>Richard Mudgett <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/9270">View Change</a></p><p>Patch set 3:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p>(9 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c">File main/cdr.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@3787">Patch Set #3, Line 3787:</a> <code style="font-family:monospace,monospace"> int nextms = 0;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">No need to initialize nextms. You always initialize it below.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@3796">Patch Set #3, Line 3796:</a> <code style="font-family:monospace,monospace"> /* manually reschedule from this point in time */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Comment isn't quite right anymore. How about:</p><p style="white-space: pre-wrap; word-wrap: break-word;">Calculate the next scheduled interval.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@3799">Patch Set #3, Line 3799:</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;"> if (!nextms) {<br> /* I hate doing this, but previously if you set the batch time to 0, it would continuously<br> resubmit batches to the scheduler. IMHO that seems buggy, but I'd rather not break it<br> if someone was relying on that behavior */<br> nextms = 1;<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'd say it is buggy. All they are doing is needlessly loading their CPU when CDR's already load the CPU enough. The user should not be allowed to set it to zero. I'd say set nextms to 1000 and fix the allowed range of the "time" option in process_config() to 1 - MAX_BATCH_TIME.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@3807">Patch Set #3, Line 3807:</a> <code style="font-family:monospace,monospace"> /* returning zero so the scheduler does not automatically reschedule */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This comment is not needed anymore as you never return zero now.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@3884">Patch Set #3, Line 3884:</a> <code style="font-family:monospace,monospace"> /* Don't call start_batch with cdr_batch_lock held */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">s/call start_batch/submit a batch/</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@3899">Patch Set #3, Line 3899:</a> <code style="font-family:monospace,monospace"> /* this only can happen if someone sets there batch time to 0 or does 'cdr submit' from the CLI back to back quickly */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Restore the previous comment as there will normally not be an empty scheduler when in batch mode. The only time I can see the scheduler being empty here is if someone is rescheduling a batch in start_batch_mode().</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@3910">Patch Set #3, Line 3910:</a> <code style="font-family:monospace,monospace"> ast_debug(2, "Processed %d scheduled CDR batches from the run queue\n", numevents);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This debug msg isn't quite right.<br>s/scheduled //</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@4410">Patch Set #3, Line 4410:</a> <code style="font-family:monospace,monospace"> SCOPED_LOCK(batch, &cdr_batch_lock, ast_mutex_lock, ast_mutex_unlock);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This scoped lock can cause a deadlock when you call start_batch_mode().</p><p style="white-space: pre-wrap; word-wrap: break-word;">Sched task submit_scheduled_batch() calls cdr_submit_batch() which locks the cdr_batch_lock. If it is running when start_batch_mode() tries to reschedule you have deadlock.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't see why we even need to lock cdr_batch_lock here as its job is to protect access to the CDR batch list.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@4424">Patch Set #3, Line 4424:</a> <code style="font-family:monospace,monospace"> /* Kill the currently scheduled item */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This comment needs to be removed.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/9270">change 9270</a>. To unsubscribe, 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/9270"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 15 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I6333e865db7c593c102c2fd948cecdb96481974d </div>
<div style="display:none"> Gerrit-Change-Number: 9270 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: Matthew Fredrickson <creslin@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 22 Jun 2018 19:13:19 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>