[OpenSER-Devel] SF.net SVN: openser: [4329] trunk/modules/benchmark/benchmark.c

[Daniel-Constantin Mierla]

On 06/06/08 22:44, Bogdan-Andrei Iancu wrote:
> Daniel,
>
> Not sure what make you say it is a quick hack... I admit it is not the 
> final version of the idea, but this is the first step, trying to solve 
> the most urged problems - if you want to call a quick hack anything 
> that solves something, fine with me.
I said so because it solves just some specific cases, and it is not by 
far what it is intended to be. Also, with same level of effort could 
solve more.
>
> It is the first step as in the next version, transaction persistence 
> (like being able to use onreply / failure / branch  routes) it will be 
> added - right now I had no time to do the changes into TM in order to 
> store the fake sip_msg for later usage. This requires some more 
> complex changes that requires more time.
So that will be ready for production in about 1 year time, or just 
because this quick commit was done, new features related to it can be 
committed during testing phase? Are they going to be in 1.4 or 1.5? It 
was not clear for me so far, I based my conclusion on project policy 
that new functionalities are not added during freezing phase, so I 
believed it is 1.5.


>
> Also, see me inline comments on the your technical remarks.
>
> Regards,
> Bogdan
>
>
> Daniel-Constantin Mierla wrote:
>>
>>
>> On 06/06/08 21:04, Bogdan-Andrei Iancu wrote:
>>> Hi Henning,
>>>
>>> What do you mean by "we have to live with it" ? it sounds like 
>>> something terrible wrong happened.
>>>
>>> To be honest  I fail to see the logic behind of this. You and Daniel 
>>> are complaining about some new code - based on a superficial 
>>> understanding of the code and functionality you start formulating 
>>> some gratuitous  opinion, but without bringing any real technical 
>>> arguments for this opinion.
>>>
>>> I consider truly fair and respectful for a collective effort to :
>>>    - try understand what is the idea and the code before expressing 
>>> any disapproval.
>>>    - if you are not able to understand, please ask the developer to 
>>> clarify the issues for you - better that making wrong assumptions.
>> If you do read my previous email you will understand what I meant. 
>> This new stuff looks like quick hack to fix just few demands, whilst 
>> a discussion about such major change (done in the last minute) would 
>> reveal a better design and approach. openser is now beyond one man 
>> interests, and as you talk about collective work and collaboration, 
>> what happened it is not the right approach.
> [bogdan]
> If you have/had a better solution, nobody stopped you to implement it 
> ;) . The idea about this route was advertised and discussed for some 
> time over the tracker and mailing list. Unfortunately there was no 
> input from your side.
There was no request for comment for such new route - in various 
contexts can pop up ideas, but in the past they were started as new 
threads. Nevertheless, such last minute commit pushes the developers 
community and all that rely on the project, giving no chance to analyze 
and contribute. Everyone is stuck one year because cannot make use of 
the local_route with their code as it is now or eventually improved 
version due to public discussion and contributions.

And I may have some implemented, because it is bug that siptrace cannot 
get everything stored -- that has to be solved in a way or other. 
Project policy allows me that :-) -- and will be available only for 
siptrace.


>
>>
>> I do understand all the code, and really don't like the solution. 
>> What it brings:
>> - for internally generated messages we can now: do xlog, add headers, 
>> benchmarking, avpops and accounting
> [bogdan]
> these were the first functionality enabled for this route. As said, 
> the work regarding this route is not yet completed. For the moment is 
> brings solution for the most demanded features that were discussed on 
> tracker and mailing list.
>>
>> The new route is executed after the buffer with the message (that was 
>> previously sent to the network) is built,
> [bogdan]
> actually the route is executed before sending anything on the net - 
> any processing after that makes no sense.
Let me rephrase so you get it right this time: previously, before this 
commit, the buffer with the local generated message was built and sent 
to the network. Now, after it is built, it is parsed in a local sip_msg, 
local route is executed, message buffer rebuilt again with changes 
incurred from local_route and then sent. Hope I could explain it better 
this time.

>> reparsed in a local static sip_msg structure, and then apply 
>> eventually new changes induced from local_route, building a new 
>> buffer and sending. No real connection to TM, just that the function 
>> to send such message is there
> [bogdan]
> that is the whole idea and purpose - the route is called when TM sends 
> an internal request. I think it is quite a strong connection :)

Half way again, not all sent by tm are caught there, and should be named 
tm_local_route as there can be siptrace_local_route for the replicated 
messages sent by siptrace to enable there adding headers, etc -- 
confusion will be avoided.

>> -- no possibility to bind to transaction callbacks, or other stuff 
>> specific to TM.
> [bogdan]
> it is not yet there, but (as said) it will be in the next version. 
> Simple, it was not time for this complex task.
>>
>> Same, but more will be available if it is going to be plugged in the 
>> core, for all messages.
> [bogdan]
> as the route is intended only for internally generated requests, TM is 
> the only one doing it, so is will be useless for core - the core does 
> not provide no functionality to send requests/replies without script 
> interaction. And if you have script interaction, there is no need for 
> another special route.

Quite wrong, siptrace sends messages with core functions (so the 
functionality is there, in core), replicating what it stores locally. 
And adding some extra headers for those messages will be very useful.

>
>> Self generated messages are easy to detect, by that we can select the 
>> ones that are processed with the current version. More over:
>> - siptrace can be easily updated to avoid tm callbacks and use this 
>> new route for outgoing messages, and it will work even for stateless 
>> mode
>> - the destination ip/port/protocol can be made availabe in just few 
>> lines of code, allowing flexible filtering and accounting (there is a 
>> pending feature request IIRC)
>> - details from sent buffer may be accounted
> [bogdan]
> all this proves me that unfortunately you missed the point with this 
> route.

Not at all, I really understand it, and might be useful, but should be 
introduced later, when all bindings to tm and other planned features are 
to be implemented before a major release. As it is now, it fits better 
as send_route, for all messages, enabling lot more feature, at no cost.

A short request for comment on the mailing list might have reveal even a 
better approach, there are more heads there.

> This route does not intend to provide access to message buffer or 
> other info filled before putting the message on the net (as you 
> suggest). It simple provides script access to requests internally 
> generated by TM (which otherwise are transparent for the script).
> Maybe the fact that the route is building a sip_msg (to provide it to 
> the script functions) from the script tricked you - but when TM builds 
> a new brand requests, it directly builds the buffer without no sip_msg.
>
> siptrace does not generate any new request - it is just dumping the 
> message buffer.
A diving in the code will reveal you that it is sending a replica of the 
stored message to a configured address -- off course, there are details 
in the readme:

http://www.openser.org/docs/modules/1.3.x/siptrace.html#AEN129

> destination ip/port/protocol - not related to any of this.
> sent buffer - again, it is not the purpose of this route.

This route (with the ideas behind it) I see it usable in production with 
the next release. The current code could have been used for much more.

All the other developers have no chance to see how this route bring 
benefits to their code or modules. Looks like only your decision which 
functions to be enabled or not for local_route, because the rest have no 
time before freezing.

For such route, I see no reason why I cannot use dispatcher 
(lcr/carrierroute/...) to load balance the presence messages pushed via 
MI (just as an example).

A route where I cannot change destination, should be clearly named to 
suggest that. I see it like one year we have to deal with lot of 
questions and explanation why this route is actually intended to let one 
process local generated messages more or less like the ones received, 
but for a while you can do just 4-5 operations, no load balancing, no 
re-routing, a.s.o.

When it was decided that the major release to be prepared before the 
summer, I was one of those suggesting it is not enough time. Right now I 
am more convinced about that. I postponed things I committed to do just 
because there was no time to really discuss about (merging some modules, 
a common interface for radius/diameter...). Looks like a big potential 
improvement, but pretty much useless (comparing with what can be done 
there) for one year. With one more week, we can speed up deployment of 
very useful functionalities in production with this release.

Therefore, I am going to rise against freezing this night, proposing a 
public vote about, asking one more week to reflect about all new stuff. 
I am going to send a new email about and I will obey decision taken there.

Cheers,
Daniel


>
> As, said, my feeling is that you made some confusion on the route 
> purpose - it is not intended to provide access to the buffer or IP 
> info just before pushing the message on the net - it simple provide 
> script access for some requests that didn't had any; it is not about 
> any buffer, as the route provides a sip_msg to the functions.
>> - the self generated CANCELs can be handled as well.
> [bogdan]
> yes - already discussed with Carsten Bock and this will be covered by 
> the next version (see my email to him) - internal generated CANCELs 
> are bit too delicate to catch and process in a route (as they are part 
> of the INVITE transaction)
>> Just some that got in my mind.
>>
>>>
>>>
>>> Again, I open to any technical comments on the code, if any.
>>
>> You mean about the C code or? Nobody claimed current code is wrongly 
>> written, just it is not the proper solution for project's demands.
> [bogdan]
> how do you define the demand of the project ? because this new route 
> was a direct result of needs of the several people. Usually I tent to 
> consider the users needs as the project needs.
> If wrong, any input is welcome.
>>
>> Cheers,
>> Daniel
>>>
>>> Regards,
>>> Bogdan
>>>
>>>
>>> Henning Westerholt wrote:
>>>> On Friday 06 June 2008, Daniel-Constantin Mierla wrote:
>>>>  
>>>>> [..]
>>>>> I have to second Henning here in regard to a proper discussion may 
>>>>> lead
>>>>> to better solution. Maybe I am missing something, but from what I 
>>>>> have
>>>>> seen in the description and the code, binding this route to TM solve
>>>>> just few of the demands. It is called after the buffer of the 
>>>>> message to
>>>>> be sent is built.
>>>>>
>>>>> IMO such route shall be available for all messages sent by 
>>>>> openser, so
>>>>> we get access in it to what is going to be sent on the network. There
>>>>> are solutions to detect that the message is locally generated (e.g.,
>>>>> only one via header), to decide there what to do. Apart of the 
>>>>> benefits
>>>>> brought now, siptrace will work for all messages, we can get make
>>>>> available the destination ip and port, therefore filtering is more
>>>>> flexible, also logging/accounting get access to this information.
>>>>>
>>>>> I believe that this is a real topic for discussion, and if it is 
>>>>> really
>>>>> important, we can postpone one or two days the freeze for the right
>>>>> decision. Leaving a half solution for a full release is not a right
>>>>> approach.
>>>>>     
>>>>
>>>> Hello Daniel,
>>>>
>>>> i agree here to you, of course. But i think that further delaying 
>>>> of the freeze is also not a good solution. So i think we'll 
>>>> probably need to live with this solution for 1.4.
>>>>
>>>> Cheers,
>>>>
>>>> Henning
>>>>
>>>>
>>>>   
>>>
>>
>

-- 
http://www.asipto.com