Re: Patch: EMI UUCP DLRs (final)

Hi,

All testing is valuable. You don't need more than 1 smsc, since patch was 
against destination only, and the queries already use the smsc as key. I am 
not sure what you mean by connection-id (smsc-id? system-id? username?), but 
if you use only 1 smsc, I don't think it will be a problem. Beware that in 
dlr table entries will be the same as in svn kannel. AFAIK all chars you 
have are a valid smsc-id , as long as you don't use ";" or spaces.

Thanks,
Nikos
----- Original Message ----- 
From: "Kyriacos Sakkas" <[email protected]>
To: "Nikos Balkanas" <[email protected]>
Cc: <[email protected]>
Sent: Friday, July 16, 2010 12:42 PM
Subject: Re: Patch: EMI UUCP DLRs (final)


> 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] **