Fixes Restcomm#150 : Resubmissions of requests to alternate peers now possible until all peers exhausted, while respecting Weighted Round Robin balancing algorithm #156
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merge pull request RestComm#146 from StephenDwyer-kcom/master
…ing as expected
…r class; changes require around peer selection and should move away from using CC-Request-Number AVP for signifying resubmissions. Interim commit only to manage change
…iting behaviour in WeightedRoundRobinRouter class. Tests added to validate WeightedRoundRobinResubmittingRouter selects previously unattempted peers for a resubmission of an existing message, based on existing weighted round robin algorithm.
…tting requests which have received a Busy or Unable to Deliver Answer from one peer, to an alternative peer, and to avoid perpetual re-submission of such requests
| isProcessed = message == null; | ||
| } | ||
| avpResCode = message.getAvps().getAvp(RESULT_CODE); |
There was a problem hiding this comment.
I'm wondering now whether, because if a redirect was processed, the message (i.e. Answer) could possibly be a different message to when avpResCode was originally assigned its value, this assignment statement should be added again, to ensure the avpResCode contains the most recent value, and this particular change reverted?
This reverts commit 2575d51.
This was referenced Jun 9, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #150
The IRouter interface now has a new method, processBusyOrUnableToDeliverAnswer(IRequest request, IPeerTable table) which can be called to send a request, which initially received a 3002 or 3004 response from one peer, to an alternate peer.
The PeerImpl class now has a helper method, isBusyOrUnableToDeliverAnswer(Avp avpResCode, IMessage answer), to unambiguously determine whether a response can be considered a Busy or Unable to Deliver response (quite simply whether the response was 3002 or 3004). A second method has also been added to this class, processBusyOrUnableToDeliverAnswer(IRequest request, IPeerTable table), which simply delegates resubmission to the IRouter's new processBusyOrUnableToDeliverAnswer(IRequest request, IPeerTable table) method, when the Router Implementation supports this (see below [^1] for details).
In the PeerImpl's connect() method, the above two methods are used to determine whether a response was a Busy or Unable to deliver response, and, when this is the case, delegates resubmission to the IRouter's new processBusyOrUnableToDeliverAnswer(IRequest request, IPeerTable table) method.
In RouterImpl, an overloaded method selectPeer(IMessage message, List availablePeers) has been added, to allow the default RouterImpl and subclasses (WeightedRoundRobinRouter & WeightedLeastConnectionsRouter) to resend the original request after selecting an alternate peer. In the default PeerImpl and the other existing subclasses, this overloaded method simply calls selectPeer(List availablePeers), in order to maintain the original behaviour.
A new Router class has been defined, WeightedRoundRobinResubmittingRouter, into which the actual functionality has been implemented. This has been copied from WeightedRoundRobinRouter. This new class was defined in order to protect existing users from any possible undesirable side-effects from changes in routing functionality. So, in order to have Busy or Unable to Deliver responses retried on alternate peers, the WeightedRoundRobinResubmittingRouter needs to be configured as the chosen RouterEngine in the Extensions section of the jDiameter config.
The WeightedRoundRobinResubmittingRouter will select alternate peers for resubmissions, until all peers have been exhausted for any given request, while respecting the Weighted Round Robin balancing algorithm.
[^1] To allow the PeerImpl to determine whether or not any particular Router implementation supports resubmissions, a method canProcessBusyOrUnableToDeliverAnswer() has been added to the IRouter interface. This method is implemented in RouterImpl to return false by default and must be overridden by classes supporting this functionality (currently only the WeightedRoundRobinResubmittingRouter).
It may be desirable to merge the functionality into the existing Router classes, rather than to have it exclusively in the newly defined WeightedRoundRobinResubmittingRouter. Please let me know whether this should be done.
Regarding Unit Testing:
The TestRouter class has been extended with Unit Tests covering various scenarios which the WeightedRoundRobinResubmittingRouter may encounter. The existing tests remain unchanged. TestRouter now extends junit.framework.TestCase, to ensure that the tests are run every time a Maven build is performed.
A log4j.properties file has been added to allow all logging output from classes under test to be directed to a ConsoleAppender.
Furthermore:
All code has been reformatted according to the Restcomm checkstyle standards.