Hi, 2 MT at the same time are just not fixable for EMI due to the protocol flaw...
Thanks, Alexander Malysh Am 21.07.2010 um 01:00 schrieb Vincent CHAVANIS: > > I'm +0 for this patch > because i really dislike the <LIKE '%searches'>, they are *extremely* slow > but this is at least the most advanced patch that partially fixes the EMI's > dlr issue. > *but* you still have an issue if 2 MTs are send in the same second (same ts) > to the same destination. > which DLR will you take ? Assuming you're using an 'trans-id' for each MT, > you have 1/2 (or more) chance to call the wrong one (depending on how this > was inserted on your db) > I personnally experienced this on *hudges* MT pushes. > > Vincent > > > Le 20/07/2010 23:05, Alexander Malysh a écrit : >> Hi Nikos, >> >> +1 except some indents issues but I will fix it myself... >> >> Any test results from users? >> >> Thanks, >> Alexander Malysh >> >> >> Am 20.07.2010 um 12:07 schrieb Nikos Balkanas: >> >> >>> OK. I believe this is it. >>> >>> BR, >>> Nikos >>> ----- Original Message ----- From: "Alexander Malysh"<[email protected]> >>> To: "Nikos Balkanas"<[email protected]> >>> Cc: "Byron Kiourtzoglou"<[email protected]>;<[email protected]> >>> Sent: Tuesday, July 20, 2010 12:14 PM >>> Subject: Re: Patch: EMI UUCP DLRs (final) >>> >>> >>> Hi Nikos, >>> >>> see below ... >>> >>> Am 20.07.2010 um 03:03 schrieb Nikos Balkanas: >>> >>> >>>> Hi Alex, >>>> >>>> Please see inlined comments: I am still waiting feedback from some users >>>> testing it. >>>> >>>> >>>>> ----- Original Message ----- From: "Alexander Malysh"<[email protected]> >>>>> To: "Nikos Balkanas"<[email protected]> >>>>> Cc: "Byron Kiourtzoglou"<[email protected]>;<[email protected]> >>>>> Sent: Tuesday, July 20, 2010 1:14 AM >>>>> Subject: Re: Patch: EMI UUCP DLRs (final) >>>>> >>>> >>>> >>>>> Hi Nikos, >>>>> >>>> >>>>> I'm back from vacation and here are comments to your patch. Patch looks >>>>> OK but still some things to fix: >>>>> >>>> >>>>> --- gw/dlr_mem.c (revision 4833) >>>>> +++ gw/dlr_mem.c (working copy) >>>>> @@ -125,8 +125,28 @@ >>>>> /* XXX: check destination addr too, because e.g. for UCP is not enough >>>>> to check only >>>>> * smsc and timestamp (timestamp is even without milliseconds) >>>>> */ >>>>> - if(octstr_compare(dlr->smsc,smsc) == 0&& >>>>> octstr_compare(dlr->timestamp,ts) == 0) >>>>> + if (dst){ >>>>> + Octstr *dst_min; >>>>> + int len1 = octstr_len(dlr->destination), len2 = octstr_len(dst); >>>>> + >>>>> + if (len1< len2) >>>>> + return(1); >>>>> + >>>>> + dst_min = octstr_duplicate(dlr->destination); >>>>> + if (len1> len2) >>>>> + octstr_delete(dst_min, 0, len1 - len2); >>>>> + >>>>> + if (octstr_compare(dlr->smsc, smsc) == 0&& octstr_compare(dlr-> >>>>> + timestamp, ts) == 0&& octstr_compare(dst_min, dst) == 0) { >>>>> + octstr_destroy(dst_min); >>>>> return 0; >>>>> + } >>>>> + octstr_destroy(dst_min); >>>>> + } >>>>> + else >>>>> + if (octstr_compare(dlr->smsc, smsc) == 0&& octstr_compare(dlr-> >>>>> + timestamp, ts) == 0) >>>>> + return 0; >>>>> >>>> >>>>> why so complicated? >>>>> if (dst) { >>>>> pos = octstr_len(dlr->destination) - octstr_len(dst); >>>>> if(pos< 0) >>>>> pos = 0; >>>>> if (octstr_compare(dlr->smsc, smsc) == 0&& >>>>> octstr_compare(dlr->timestamp, ts) == 0&& >>>>> octstr_search(dlr->destination, dst, pos) !> = -1) >>>>> return 0; >>>>> } else ... >>>>> >>>> Same degree of complexity. You use octstr_search, I use octstr_compare. >>>> Personally I wouldn't use either, but octstr_search is slightly faster. >>>> The only reason I used octstr_delete& compare is because you told me to >>>> use truncate on a copy, and I even asked you twice about it. Will change >>>> it. >>>> >>> It's not the same. In my version you don't need duplicate, destroy etc. >>> and it just simpler to read... >>> >>> >>>> >>>>> --- gw/dlr_sdb.c (revision 4833) >>>>> +++ gw/dlr_sdb.c (working copy) >>>>> + if (dst) >>>>> + like = octstr_format("AND \"%S\" LIKE '%%%S' %s", >>>>> fields->field_dst, >>>>> + dst, sdb_get_limit_str()); >>>>> + else >>>>> + like = octstr_imm(sdb_get_limit_str()); >>>>> >>>> >>>>> why not keep sdb_get_limit() in place and do it as for other DBs? >>>>> and \"%S... seems to be wrong... >>>>> >>>> I don't understand what you are asking for here. The code was like that >>>> before and I didn't change it. \"%S\" is a typo. Will fix. >>>> >>> why did you put sdb_get_limit_str() here? it should be keep in sql = >>> octstr_format("select ... %s", sdb_get_limit_str, ...)... >>> >>> >>> >>>> >>>>> --- gw/dlr.c (revision 4833) >>>>> +++ gw/dlr.c (working copy) >>>>> Msg *msg = NULL; >>>>> struct dlr_entry *dlr = NULL; >>>>> + Octstr *dst_min = NULL; >>>>> >>>> >>>>> indents.... >>>>> >>>> >>>>> + if (use_dst&& dst) { >>>>> + dst_min = octstr_duplicate(dst); >>>>> + int len = octstr_len(dst); >>>>> + >>>>> + if (len> MIN_DST_LEN) >>>>> + octstr_delete(dst_min, 0, len - MIN_DST_LEN); >>>>> + } >>>>> >>>> I see. Intends are 3 spaces, instead of 4. That's why I don't use spaces. >>>> I tried my best, but please feel free to adjust them exactly the way you >>>> like them. >>>> >>> :) kannel uses 4 spaces instead of tabs... >>> >>> >>>> >>>>> indents... >>>>> >>>> >>>>> /* destroy struct dlr_entry */ >>>>> dlr_entry_destroy(dlr); >>>>> + octstr_destroy(dst_min); >>>>> >>>> >>>>> ditto >>>>> >>>> >>>> BR, >>>> Nikos >>>> --- gw/dlr.h (revision 4833) >>>> +++ gw/dlr.h (working copy) >>>> +Msg* dlr_find(const Octstr *smsc, const Octstr *ts, const Octstr *dst, >>>> int type, >>>> + int use_dst); >>>> >>>> change to >>>> +Msg* dlr_find(const Octstr *smsc, const Octstr *ts, const Octstr *dst, >>>> int type, int use_dst); >>>> >>>> >>>> Thanks, >>>> Alexander Malysh >>>> >>>> Am 14.07.2010 um 14:38 schrieb Nikos Balkanas: >>>> >>>> >>>>> 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> >>>>>>> >>>>>> >>>>> <kannel.diff> >>>>> >>>> >>> <kannel.diff> >>> >> >> > >
