@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 or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2363#issuecomment-651667876
_______________________________________________
Kamailio (SER) - Development Mailing List
sr-dev@lists.kamailio.org
https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev

Reply via email to