@Kalki70 - I think you start crossing the common sense line with your last
comment on a personal attack. As I said my non-technical remark was more from
an admiration point of view -- bugs that are found in code older than 15 years
deserve somehow an "highlighting prize", but it is not the case here, you wrote
you FIX current implementation, proves to be a new set of distribution
algorithms. Everything else was technical review and comments about the code
changes. If you want the kind of discussions with personal attacks, Kamailio is
not a project for such approach.
The way I proposed to add the feature has many benefits:
* cleaner enhancement, the existing algorithms not being affected at all
* ability to use at the same time with other algorithms -- while you may want
to do your distribution to transcoding servers, maybe the registrations are
better with old style algorithm to know better where user end up registered on
a server maintenance time frame
* backward compatibility
* follows the same approach with weight and rweight algorithms
Yours keeps the backward compatibility, nothing more adds to the decision
complexity of setting modparams.
Besides the above aspects, I also had the specific code review remarks:
declaration of variables at the beginning of blacks and the not-so-good second
hashing which is done over an integer printed as string. For the second here,
the scope of hashing is to get an integer from string, if you have already the
integer, converting it to string to hash again is not really a good solution.
Even from distribution point of view, if you have 3 server addresses in the
set, then you will always has either "0", "1" or "2". Hashing 3 string values
will return always 3 integers (hash id). I haven't spent time on analyzing much
the code, but practically, if server 2 is down, then all traffic sent to it
will be sent to the server corresponding to the hash id computed from "1", so
you still send to another **single** server. Second hashing has to be done on
something that can ensure better distribution.
As code issues in the patch, one seems to be a potential infinite loop in the
while (( maxRehash > 0 ) && ds_skip_dst(idx->dlist[fullHash % idx->nr].flags )
Code indentation is also not following the style in the file. And those were
what I could spot quickly so far.
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
Kamailio (SER) - Development Mailing List