Fix for issue#36 - loadbalancing and failover extension implemented#82
Fix for issue#36 - loadbalancing and failover extension implemented#82gfigiel wants to merge 3 commits intoRestComm:masterfrom
Conversation
brainslog
left a comment
There was a problem hiding this comment.
@gfigiel, thanks for this great contribution. I would ask you if you could break this into smaller, more scoped PRs, as it is hard to review with too many changes and some unrelated to the PR topic (eg: removing "~" files, fixing documentation issues, etc) and try to avoid unnecessary changes (indentation, import changes, else/catch placement, etc) as they hide the actual changes, please do those that apply at a separate PR and they can be quickly merged (thanks for spotting them!)
Also, would be good to provide some more context on what changed, how to use it, a summary of the modifications and some explanation to it to make the review easier.
With regards to license headers, for new files, please use the Telestax license header (as in
Some general comments from a quick review (sorry if I misunderstood anything, on the scoped PRs should be easier to keep up) but there seems to be some things which are application specific (like the added org.jdiameter.api.RequestType which is CCA/Ro specific but added at general API, the TX_TIMEOUT parameter which is also CCA/Ro specific but added to general configuration) and some other stuff that should be more generic and it's implemented as part of the App Session (shouldn't the retransmission timer be made at a higher level and handled by the stack core instead of App session?)
I will still take some more time to grasp all the changes, but just letting you know these in advance so if you could go with breaking and cleaning the PR, that would be great and allow for more detailed discussion.
Thanks! Awesome contribution!
|
Commit c92964a contains:
|
|
Can you please reduce the scope of this PR to the changes regarding the functionality we are trying to implement with it ? There's too much noise with 3,362 additions and 32,441 deletions for proper review and merge. I'll be happy to review and merge the other fixes you have made along, but in separate PRs. Also, if you could try to explain better what you are trying to achieve and address some of the technical questions I've posted, would be great. Thanks! |
|
Closing as there's a new Pull Request on #97 |
Loadbalancing and failover extension implementation + checkstyle corrections for issues indicated during build & master branch merge.