Hi All, It might be possible for me to do some testing with the mysql dlr storage.
One point is that at least on my system the connection ID for CIMD (smsc) used for the dlr store is not the name set in the conf file but rather a long string: "CIMD2:server.ip:server.port:login" . This can be problematic if two connections on different IPs or ports exist for the same service. My planned solution was to use a LIKE statement here as well and use only the "login" part somehow, which in my case at least happens to be unique per service and the same on multiple connects for the same service. Regards, Kyriacos Sakkas On 16/07/2010 11:43, Nikos Balkanas wrote: > Hi Byron, > > Any news? Were you able to test? > > BR, > Nikos > ----- Original Message ----- From: "Nikos Balkanas" <[email protected]> > To: "Alexander Malysh" <[email protected]>; "Byron Kiourtzoglou" > <[email protected]> > Cc: <[email protected]> > Sent: Wednesday, July 14, 2010 3:38 PM > Subject: Re: Patch: EMI UUCP DLRs (final) > > >> Here it is. Added support for dest also for cimd2 as reported by Byron. >> >> I have tested thoroughly dlr_mem.c, but only compilation for DBs, >> since i >> have no access to them. >> >> @Byron: Could you please test patch for CIMD2 against some of the DBs >> you >> use? >> >> Thanks, >> Nikos >> ----- Original Message ----- From: "Alexander Malysh" >> <[email protected]> >> To: "Nikos Balkanas" <[email protected]> >> Cc: <[email protected]> >> Sent: Wednesday, July 07, 2010 6:11 PM >> Subject: Re: Patch: EMI UUCP DLRs (final) >> >> >> Hi, >> >> sorry for delay... I have limited inet access now... see answers bellow. >> >> Am 30.06.2010 um 19:30 schrieb Nikos Balkanas: >> >>> Hi, >>> >>> Please see inlined answers. >>> Thanks for the comments and corrections. Please confirm a few remaining >>> choices. >>> >>> BR, >>> Nikos >>> ----- Original Message ----- From: "Alexander Malysh" >>> <[email protected]> >>> To: "Nikos Balkanas" <[email protected]> >>> Cc: <[email protected]> >>> Sent: Wednesday, June 30, 2010 12:02 PM >>> Subject: Re: Patch: EMI UUCP DLRs (final) >>> >>> >>>> Hi, >>> >>>> IMO we don't need to handle no destination case in DLR lookup but >>>> maybe >>>> it's not a wrong idea to be able to ignore destination for some >>>> reasons when SMSC send some junk to us. >>> >>> Not doing it, could miss some queries altogether. It would still >>> work for >>> the large majority, but could miss a few matches that would have gotten >>> otherwise. >> >> this is ok for me, we make it optional... >> >>> >>>> ok here is the review for your patch: >>>> + if (dst){ >>>> + int len = octstr_len(dst); >>>> + char *p = octstr_get_cstr(dst); >>> >>>> + if (len > MIN_DST_LEN) >>>> + p += len - MIN_DST_LEN; /* get last MIN_DST_LEN >>>> digits */ >>>> + like = octstr_create(strcat("%", p)); >>>> + gwlist_append(binds, like); >>>> + } >>> >>>> why is this in every dlr implementation? we have abstraction for this, >>>> see dlr.c >>> >>> I don't know where in dlr.c you are referring, but I see your >>> reasoning. >>> This is leftover from a previous implementation, where I passed >>> use_dest >>> in the dlr_<DB>. I considered it at the time important to pass the >>> whole >>> dest to dlr_<db> so that debug messages get the whole dest. Since I >>> started passing NULL for dest, this serves no more purpose. I will >>> abstract it in dlr_find and use octstr_delete instead. Will also >>> correct >>> debug messages accordingly in dlr_db.c. >> >> you have to abstract it in dlr_find and not repeat the same code in each >> dlr_db driver. >> >>> >>>> - sql = octstr_format("SELECT `%S`, `%S`, `%S`, `%S`, `%S`, `%S` >>>> FROM >>>> `%S` WHERE `%S`=? AND `%S`=? LIMIT 1", >>>> + if (dst) >>>> + sql = octstr_format("SELECT `%S`, `%S`, `%S`, `%S`, `%S`, `%S` >>>> FROM `%S` WHERE `%S`=? AND `%S`=? AND `%S` >LIKE ? LIMIT 1", >>>> fields->field_mask, fields->field_serv, >>>> fields->field_url, fields->field_src, >>>> fields->field_dst, fields->field_boxc, >>>> fields->table, fields->field_smsc, >>>> + fields->field_ts, fields->field_dst); >>>> ... >>>> First of all: like ? doesn't work as you expect... it should be >>>> something >>>> like: LIKE CONCAT('%%', ?) and >>> >>> Thanks. I will look into it. >>> >>>> this is too much maintenance for SQL that defined two times, how about >>>> like this: >>>> if (dst) >>>> like = octstr_format('LIKE CONCAT('%%', ?)' ...); >>>> else >>>> like = octstr_create(""); >>> >>> You probably mean: >>> >>> like = octstr_create("=?"); >> >> no, I mean octstr_create("") or better use octstr_imm("") >> >>> >>>> sql = ...(".... %S", like) >>> >>> This is a classical maintenance vs overhead. Malloc is expensive, much >>> more so in Linux than in Solaris. Furthermore, sql="%s%S" is more >>> difficult to read and understand, since SQL mechanism is not >>> explicit, but >>> hidden in variables. Do you really want that? >> >> yes, because maintenance is then easier and malloc overhead is in >> linux not >> so much expensive as you think because glibc has preallocated memory >> pools >> and not always is system call >> needed. >> >>> >>>> The same is for DELETE, UPDATE... >>> >>>> + if (like) octstr_destroy(like); >>> >>>> octstr_destroy will check for NULL for you... >>> >>> I am aware of that, but it costs a function call and a few more >>> statements...Either way is fine. I can change it. >> >> function call is not really issue against code readability... and if >> this is >> really your argument then convert all calls to if (..!=NULL) bla... >> (just a >> joke) >> >>> >>>> + if (octstr_compare(dlr->smsc, smsc) == 0 && >>>> octstr_compare(dlr-> >>>> + timestamp, ts) == 0 && memcmp(p1 + len1 - size, p2 + >>>> len2 - >>>> size, >>>> + size) == 0) >>>> + return 0; >>> >>>> memcmp??? why not just use truncated destination and do: >>>> octstr_search??? >>> >>> Actually octstr_truncate won't work since it truncates from the end. >>> octstr_delete, would work, however, destroying the original Octstr >>> in the >>> process, unless I duplicated them. It would need to be done on both >>> destinations to work. Code would be more, and the malloc, free and >>> copy, >>> have an overhead. Memcmp doesn't change the original Octstr and is >>> natural >>> for such operations. However, it is out of kannel style, so you have >>> every >>> right to ask me to change it. Do you? >> >> yes, please change it. >> >>> >>>> +Msg *dlr_find(const Octstr *smsc, const Octstr *ts, const Octstr >>>> *dst, >>>> int typ, int use_dst) >>> >>>> you don't need to change function. Just use dst = NULL and check it. >>> >>> No. dst is needed for debug msgs inside dlr_find. Furthermore, use_dst, >>> currently is set only for EMI. Decision is made at driver level so >>> it can >>> easily change to an smsc configuration variable if needed. >> >> ok, maybe you are right. What other people think about this ? >> >>> >>> Am 26.06.2010 um 10:23 schrieb Nikos Balkanas: >>> >>>> Hi, >>>> >>>> I believe I have accounted for almost all your comments to produce the >>>> final working patch. I have no way of testing, other than compilation. >>>> >>>> This patch will povide: >>>> >>>> 1) Align dlr_oracle.c with the rest. Currently, Oracle does a full dst >>>> find/remove/update on each DLR. >>>> 2) Dst use on find/remove/update is controlled in dlr_find by a single >>>> variable, use_dst. This is currently set only at driver level and >>>> only in >>>> the emi driver, while everyone else has it false. However, very >>>> easily, >>>> if need arises, it can be set in smsc configuration. >>>> 3) All DLR handling for all smscs will remain as it used to be till >>>> now. >>>> Only emi handling changes, hopefully for the better (that is the >>>> purpose >>>> of the patch :-)). >>>> 4) It will try to match the last 7 digits of the destination or the >>>> length of the destination if it smaller. This is defined in >>>> gw/dlr_p.h as >>>> MIN_DST_LEN. Didn't want to make it larger, wanted to avoid prefix >>>> territory at all costs, since I have seen a lot of mangling there >>>> by the >>>> SMScs. Besides, I believe that 7 digits give enough resolution >>>> 5) People using emi, should rebuild their indeces, especially if >>>> they are >>>> running large batch jobs through EMI. The LIKE % construction is >>>> not very >>>> efficient. >>>> >>>> Enjoy, >>>> Nikos >>>> <kannel.diff> >>> >> -- Kyriacos Sakkas Development Team Netsmart Tel: + 357 22 452565 Fax: + 357 22 452566 Email: [email protected] http://www.netsmart.com.cy Taking Business to a New Level! ** Confidentiality Notice: The information contained in this email message may be privileged, confidential and protected from disclosure. If you are not the intended recipient, any dissemination, distribution, or copying of this email message is strictly prohibited. If you think that you have received this email message in error, please email the sender at [email protected] **
