@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 line:

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, view it on GitHub, or unsubscribe.