[Asterisk-code-review] Change in asterisk[master]: cdr/cdr_odbc: Added to record new columns add on CDR 1.8 Ast...

[Matt Jordan (Code Review)]

Matt Jordan has posted comments on this change.

Change subject: cdr/cdr_odbc: Added to record new columns add on CDR 1.8 Asterisk Version
......................................................................


Patch Set 1: Code-Review-1

(2 comments)

https://gerrit.asterisk.org/#/c/144/1//COMMIT_MSG
Commit Message:

Line 7: cdr/cdr_odbc: Added to record new columns add on CDR 1.8 Asterisk Version
      : 
      : New columns:
      :  * peeraccount
      :  * linkedid
      :  * sequence
      : 
      : Change-Id: Ibe0c7540a88305c6012786f438a0813ad8b19127
1. Please file an ASTERISK issue for this change. This change would affect existing users, and it needs to be tracked in the issue tracker.

2. Please reference the ASTERISK issue in this commit message.


https://gerrit.asterisk.org/#/c/144/1/cdr/cdr_odbc.c
File cdr/cdr_odbc.c:

Line 82: 	if (ast_test_flag(&config, CONFIG_LOGUNIQUEID)) {
       : 		snprintf(sqlcmd,sizeof(sqlcmd),"INSERT INTO %s "
       : 		"(calldate,clid,src,dst,dcontext,channel,dstchannel,lastapp,"
       : 		"lastdata,duration,billsec,disposition,amaflags,accountcode,"
       : 		"peeraccount,linkedid,sequence,"
       : 		"uniqueid,userfield)"
       : 		"VALUES ({ts '%s'},?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)", table, timestr);
       : 	} else {
       : 		snprintf(sqlcmd,sizeof(sqlcmd),"INSERT INTO %s "
       : 		"(calldate,clid,src,dst,dcontext,channel,dstchannel,lastapp,lastdata,"
       : 		"duration,billsec,disposition,amaflags,accountcode,"
       : 		"peeraccount,linkedid, sequence)"
       : 		"VALUES ({ts '%s'},?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)", table, timestr);
This is a backwards incompatible change. Yes, it should have been done when 1.8 introduced those columns, but given the availability of cdr_adaptive_odbc, usage of cdr_odbc is generally discouraged.

Even still, breaking mid-release is not very nice.

I'd recommend one of the following:
1) Only do this change against master.
2) Add a configuration option that optionally enables updating these fields. If people's database schemas support those columns, then they can enable the option and update them accordingly. If not, they are unaffected by the change. The option should default to False.

In either case, the UPGRADE file needs to be updated to reflect this change.

If you do decide to add a configuration option, then this patch needs to be cherry-picked to the other supported branches (11, 13)


-- 
To view, visit https://gerrit.asterisk.org/144
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe0c7540a88305c6012786f438a0813ad8b19127
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Rodrigo Ramirez Norambuena <decipher.hk at gmail.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes