30 Jun
2020
30 Jun
'20
12:13 p.m.
@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