[asterisk-dev] [Code Review] 3057: CDRs: Synchronize dialplan access through Stasis; clean up Answer and DISA

[Matt Jordan]

> On Dec. 12, 2013, 11:48 a.m., opticron wrote:
> > /branches/12/funcs/func_cdr.c, lines 472-474
> > <https://reviewboard.asterisk.org/r/3057/diff/2/?file=49301#file49301line472>
> >
> >     Return and reduce indentation.
> 
> Matt Jordan wrote:
>     Normally I'd agree, but that approach has one drawback here.
>     
>     Providing block scope to the code path that requires a Stasis subscription lets me RAII_VAR the subscription such that it is only allocated/cleaned up if we are operating on a channel. The code path that performs a direct read callback actually gets hit a lot - all of the adaptive CDR backends use that approach to write out CDR records. Saving ourselves 16+ allocations on each CDR write is a good thing.
>     
>     If I check the channel name length and return, I'd still want block scope for the subscription - which means I don't actually reduce indentation here at all.
> 
> opticron wrote:
>     The allocation is already broken out from the RAII_VAR and can stay with the code it is relevant to which would prevent undue allocations while still affording the benefits of RAII_VAR. The end result would be moving the RAII_VAR with NULL initializer up to the top of the function.
> 
> Matt Jordan wrote:
>     RAII_VAR still imposes a penalty - you still execute the destructor callback when you don't need to.

And, since this is a single indentation in an otherwise non-indented section of code, the benefit of eliminating a single indentation is miniscule at best.

We're not talking three nested if statements here.


- Matt


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


On Dec. 12, 2013, 12:03 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3057/
> -----------------------------------------------------------
> 
> (Updated Dec. 12, 2013, 12:03 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-22884 and ASTERISK-22886
>     https://issues.asterisk.org/jira/browse/ASTERISK-22884
>     https://issues.asterisk.org/jira/browse/ASTERISK-22886
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> When doing the rework of the CDR engine that pushed all of the logic into cdr.c and made it respond to changes in channel state over Stasis, I knew that accessing the CDR engine from the dialplan would be "slightly" non-deterministic. Dialplan threads would be accessing CDRs while Stasis threads would be updating the state of said CDRs - whereas in the past, everything happened on the dialplan threads. Tests have shown that "slightly" is in reality "very".
> 
> This patch synchronizes things by making the dialplan applications/functions that manipulate CDRs do so over Stasis. ForkCDR, NoCDR, ResetCDR, CDR, and CDR_PROP now all use Stasis to send their requests over to the CDR engine, and synchronize on the Stasis subscription so that they return their values/control to the dialplan at the appropriate time.
> 
> While going through this, the following changes were also made:
>  * DISA, which can reset the CDR when a user successfully authenticates, now just uses the ResetCDR app to do this. This prevents having to duplicate the same Stasis synchronization logic in that application.
>  * Answer no longer disables CDRs. It actually didn't work anyway - calling DISABLE on the channel's CDR doesn't stop the CDR from getting the Answer time - it just kills all CDRs on that channel, which isn't what the caller would intend.
> 
> 
> Diffs
> -----
> 
>   /branches/12/main/pbx.c 403703 
>   /branches/12/main/cdr.c 403703 
>   /branches/12/include/asterisk/cdr.h 403703 
>   /branches/12/funcs/func_cdr.c 403703 
>   /branches/12/apps/app_forkcdr.c 403703 
>   /branches/12/apps/app_disa.c 403703 
>   /branches/12/apps/app_cdr.c 403703 
>   /branches/12/UPGRADE.txt 403703 
>   /branches/12/CHANGES 403703 
> 
> Diff: https://reviewboard.asterisk.org/r/3057/diff/
> 
> 
> Testing
> -------
> 
> * The CDR tests pass (now deterministicly)
> * The hangup handler tests now pass, as they can get reliable values in the h extension from the CDR engine
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

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