[asterisk-dev] [Code Review] Rework CDR API Internals to Prevent Loss of CDR Records

Fri Feb 26 01:49:59 CST 2010

> On 2010-02-25 12:22:30, Mark Michelson wrote:
> > First round of comments. I haven't looked at all the backend implementation changes yet, just the cdr.[hc] changes.

Thanks, Mark!  That's where the biggest changes are, anyway.

> On 2010-02-25 12:22:30, Mark Michelson wrote:
> > /trunk/include/asterisk/cdr.h, lines 195-200
> > <https://reviewboard.asterisk.org/r/531/diff/1/?file=8321#file8321line195>
> >
> >     Use \param for function parameters. This same comment applies to other places as well, but I'm not going to mark the individual occurrences.

Gah, I can't believe I still haven't shook that habit.  Fixed ...

> On 2010-02-25 12:22:30, Mark Michelson wrote:
> > /trunk/include/asterisk/cdr.h, line 263
> > <https://reviewboard.asterisk.org/r/531/diff/1/?file=8321#file8321line263>
> >
> >     "its"

fixed

> On 2010-02-25 12:22:30, Mark Michelson wrote:
> > /trunk/include/asterisk/cdr.h, lines 279-304
> > <https://reviewboard.asterisk.org/r/531/diff/1/?file=8321#file8321line279>
> >
> >     Fill in all the todo comments here.

I will if I have time.  These were existing functions that were all grouped together and not documented at all.  I added these doxygen blocks to make them show up in the doxygen todo list as functions that needed to be documented.

> On 2010-02-25 12:22:30, Mark Michelson wrote:
> > /trunk/main/cdr.c, lines 215-218
> > <https://reviewboard.asterisk.org/r/531/diff/1/?file=8323#file8323line215>
> >
> >     I vote, if possible, to rename this enum to something else since "cdr_wrap_cache" is already used as the name of the ao2_container in a cdr_backend.

Fair enough.  done!

> On 2010-02-25 12:22:30, Mark Michelson wrote:
> > /trunk/main/cdr.c, lines 1620-1626
> > <https://reviewboard.asterisk.org/r/531/diff/1/?file=8323#file8323line1620>
> >
> >     You're leaking a backend reference every time through this loop. You either can unref it at the end of the loop, or even better, you can unref it at the end of flush_cdr_q().

Good catch, stupid mistake, fixed with your suggestion of doing it in flush_cdr_q()

> On 2010-02-25 12:22:30, Mark Michelson wrote:
> > /trunk/main/cdr.c, lines 2010-2020
> > <https://reviewboard.asterisk.org/r/531/diff/1/?file=8323#file8323line2010>
> >
> >     You're leaking a backend reference every time through this loop.

thanks, fixed!

> On 2010-02-25 12:22:30, Mark Michelson wrote:
> > /trunk/main/cdr.c, lines 2214-2228
> > <https://reviewboard.asterisk.org/r/531/diff/1/?file=8323#file8323line2214>
> >
> >     Two comments here.
> >     
> >     If you take my advice from earlier about unreffing a cdr backend at the end of flush_cdr_q, then you'll have to change the ao2_callback flags to not use OBJ_NODATA.
> >     
> >     Second, is there any particular reason you're using OBJ_MULTIPLE here? bump_backend always returns 0.

1) I'm not sure how OBJ_NODATA is related.  That's just saying that we don't want any objects returned from this ao2_callback().  I did take your suggestion, and I also updated bump_backend() to increase the reference count before adding an entry to the task processor to run flush_cdr_q().

2) OBJ_MULTIPLE isn't needed.  I'll remove it.  I think what I was thinking was that I was trying to be more expressive that I intended the operation to operate on multiple objects.

- Russell

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

On 2010-02-25 00:14:03, Russell Bryant wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/531/
> -----------------------------------------------------------
> 
> (Updated 2010-02-25 00:14:03)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This code comes from the team/russell/cdr-q branch.  It is something I did on a couple of flights this week.
> 
> The current CDR implementation in Asterisk does nothing to ensure that every CDR record is successfully logged to configured logging destinations.  A single attempt is made to log a record.  If for some reason it fails, such as connectivity to a database being temporarily disrupted, then the records are lost.  Obviously, this is very bad in many cases where these records are used for things like billing.
> 
> The primary purpose behind these changes is to ensure that every record is successfully logged to every destination that has been configured.  If posting a CDR fails, it will be queued up for a retry later.
> 
> A new method has been added for backends to register callbacks for receiving CDR records.  For CDR modules that allow posting CDRs to multiple destinations, such as multiple DB tables or multiple text files, they now make a registration into the core for every destination.  That way, the success or failure of the attempt to each destination can be tracked.  All backends that need it have been updated to operate this way.
> 
> main/cdr.c has also been updated to use more modern Asterisk APIs.  It has also been updated with various other bits of code cleanup.
> 
> One side effect of these changes is that the code that previously implemented the batch mode has been completely removed, as that code is now deprecated.  The biggest benefit from batch mode was handling CDR posting asynchronously from channel threads.  In this code, that is the only behavior that exists.
> 
> 
> Diffs
> -----
> 
>   /trunk/UPGRADE.txt 248750 
>   /trunk/cdr/cdr_adaptive_odbc.c 248750 
>   /trunk/cdr/cdr_csv.c 248750 
>   /trunk/cdr/cdr_custom.c 248750 
>   /trunk/cdr/cdr_odbc.c 248750 
>   /trunk/cdr/cdr_pgsql.c 248750 
>   /trunk/cdr/cdr_radius.c 248750 
>   /trunk/cdr/cdr_sqlite.c 248750 
>   /trunk/cdr/cdr_sqlite3_custom.c 248750 
>   /trunk/cdr/cdr_syslog.c 248750 
>   /trunk/cdr/cdr_tds.c 248750 
>   /trunk/configs/cdr.conf.sample 248750 
>   /trunk/include/asterisk/cdr.h 248750 
>   /trunk/main/asterisk.c 248750 
>   /trunk/main/cdr.c 248750 
>   /trunk/main/features.c 248750 
>   /trunk/main/manager.c 248750 
> 
> Diff: https://reviewboard.asterisk.org/r/531/diff
> 
> 
> Testing
> -------
> 
> I have done some manual testing.  However, before I consider this done, I would like to implement some automated tests, as well as touch on more backends than what I have tested so far.
> 
> I configured cdr_custom to log CDRs to multiple files.  I made calls and verified that records were logged successfully.  I made some modifications on disk such that attempts to log to one file would fail, while the other would continue to succeed.  I made more calls and verified that the CDR queue grew for the specific file that had failures.  I fixed the problem and continued making calls.  I verified that all previously failed CDRs were posted and that both files were in sync with the number of CDRs that they had.
> 
> 
> Thanks,
> 
> Russell
> 
>