just dont send two MT's to the same device in the same second. This is the fix. delay the second part for 2 sec so they can not get the same timestamp.
On 21.07.2010, at 09:23, Alexander Malysh wrote: > 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> >>>> >>> >>> >> >> > >
