Issue2271 customer work #2420

ghjansen · 2017-08-11T22:35:53Z

This patch contains:

Fix to continue with next verb after Dial fork receives BUSY from all branches, when RC is using XMS (customer ticket #34390);
Fix to continue with next verb after Dial fork receives timeout, when RC is using XMS (customer ticket #34415);
Fix to send statusCallBack notification when dial branch rejects call (customer ticket #34390);
Fix to consider dial action instead of GetNextVerb, when dial action is configured inside the RCML (found this issue during the optimisation process of previous fixes).

… verbs. Issue #2271.

# Conflicts: # restcomm/restcomm.interpreter/src/main/java/org/restcomm/connect/interpreter/VoiceInterpreter.java

gvagenas · 2017-08-18T13:46:47Z

@ghjansen Please create github issues for each of the problems and provide:

Clear description of the problem
Description of the patch/solution

Also, please prepare unit tests to assert patches. The unit the will be similar to https://github.com/RestComm/Restcomm-Connect/blob/e367a4b3187d37caad0d4d10fa7d906e58e3d02a/restcomm/restcomm.interpreter/src/test/java/org/restcomm/connect/interpreter/GatherSpeechTest.java

ghjansen · 2017-08-18T17:56:53Z

Thank you @gvagenas ! I created the issues #2432, #2433, #2434, #2435 and #2436 following your recommendations. Issue #2436 is in my sprint and i'll start it next week. I'll ping you when ready.

ghjansen · 2017-09-06T15:05:27Z

@gvagenas finally ready. Tests are here: ae97269
I'm not convinced i used the best approach but i put a relevant effort to ensure the 4 main flows behave properly. Please let me know your considerations.

jaimecasero · 2017-09-25T13:02:25Z

restcomm/restcomm.interpreter/pom.xml

@@ -172,5 +172,12 @@
 			<scope>test</scope>
 		</dependency>

+		<dependency>
+			<groupId>org.mockito</groupId>
+			<artifactId>mockito-core</artifactId>


we should start putting test dependencies in root pom, but this is not particular to this PR

Agreed. I must admit took this from another PR 😬. I'll move to root then, ok?

i did the same for another PR, but im seeing we are all adding test dependencies per subproject, and even worse duplciating the version tag.
maybe we could add all test dependecies at root level to ensure version consistency, or at least use dependencymanagement to fix version at root level.

Moved to root using dependencymanagement .

jaimecasero · 2017-09-25T13:08:31Z

...nterpreter/src/test/java/org/restcomm/connect/interpreter/mediagroup/StopMediaGroupTest.java

+                interpreter.tell(new MediaGroupResponse<String>("stopped"), observer);
+
+                //check if vi stays still and DO NOT ask for next verb
+                expectNoMsg();


i like this test cases. i find this expectNoMsg a bit opened to changes, or different behaviors, could we add assertions on current state of Actor using "is(State)" method?

I guess that is not possible given the fact method "is(State)" is not part of the akka actor scope, so no way to invoke it from the test scenario. I'll check alternatives, i agree that checking that also would bring more credibility to the results.

have you tried using TypedActors,instead of untyped?... for test purposes think is valid and it shoudl only affect on the code that instantiate the actor in the test code, not the actor code itself...

I tried to understand how it works @jaimecasero but couldn't make it. I know VI inherits from untyped, but no ideas on how to fit it in the use case avoiding structural changes. (Does it worth the time/effort to only double check FMS state?)

nope, we can make it later...

jaimecasero · 2017-09-25T13:10:03Z

...mm/restcomm.interpreter/src/main/java/org/restcomm/connect/interpreter/VoiceInterpreter.java

@@ -443,6 +457,7 @@ private VoiceInterpreter(VoiceInterpreterParams params) {
        this.asImsUa = params.isAsImsUa();
        this.imsUaLogin = params.getImsUaLogin();
        this.imsUaPassword = params.getImsUaPassword();
+        this.msResponsePending = false;


any reason why we need flags instead of proper state and transitions?. Adding boolean flags to model FSM states will diminish the FSM pattern, and lower maintainability.

Is this a composite state of "gather"? Does FSM support composite states?

I tried hard for a pure and dry FSM-based solution. But in the complexity of VI, that was the only functional solution i could came up with. Turns out that all FSM state classes manipulates shared resources that are highly sensitive, so introducing such mechanism was not possible without conflicts that break the call flow. Of course new ideas, alternatives and suggestions are always welcome! (although is important to keep in mind that the current state works!)

ill have a look,maybe we need to add composite states to the FSM definition...

@ghjansen I can see now that our FSM framework is quite limited (basically not an FSM).

It doesnt even declare how transitions are meant to be fired by FSM depending on incoming events, its just a collection of source and target states. The code needs to specify the transition to take by passing both the event and the target state, which is quite odd for a FSM pattern.

FSM doesnt support composite states.

The code is also not separating different FSMs properly, so the common use of boolean flags to have different level of states and FSM data model.

Given this FSM status, i would certainly skip/defer any refactoring here, as it will require major effort beyond the scope of this PR.

We should certainly think about refactoring all of this with proper FSM, and potentially evaluate Squirrel as Henrique already did with RMS (all previous features supported...).

Thank you very much @jaimecasero for all your considerations and guidance! Do you think that FSM/Squirrel refactoring would be a good topic to bring to the table in our next R&D offsite? I was thinking about that... not sure.

i've been discussing FSMs with henrique, and specially in the context of RC. He wrote a good summary of RC issues with FSMs two years ago, that is mostly applicable today...

The custom FSM "framework" we are using today is certainly not meeting the minimum requirements for a proper FSM pattern, just how it enforces you to check the current state, before telling the FSM to transition to target state,instead of just firing the event is painful.

The great issue though is not the FSM pattern/lib itself, but how we have merged different FSMs into one, that will make any refactoring complex (regardles of which FSM lib we use). The good thing is that we have integration test to partly cover some of these actors behavior. Again, the FSM lib discussion is not key, but how we decouple the monolithic FSMs we have now, into smaller, independent,and cooperative FSMs...

# Conflicts: # restcomm/restcomm.interpreter/pom.xml # restcomm/restcomm.interpreter/src/main/java/org/restcomm/connect/interpreter/VoiceInterpreter.java # restcomm/restcomm.interpreter/src/test/java/org/restcomm/connect/interpreter/rcml/MockedActor.java # restcomm/restcomm.interpreter/src/test/resources/restcomm.xml

jaimecasero · 2017-09-26T08:33:21Z

...tcomm.interpreter/src/main/java/org/restcomm/connect/interpreter/VoiceInterpreterParams.java

@@ -59,7 +59,7 @@
    private String transferor;
    private String transferee;

-    private VoiceInterpreterParams(Configuration configuration, DaoManager storage, ActorRef callManager, ActorRef conferences, ActorRef bridgeManager, ActorRef smsService, Sid account, Sid phone, String version, URI url, String method, URI fallbackUrl, String fallbackMethod, URI statusCallback, String statusCallbackMethod, String referTarget, String emailAddress, ActorRef monitoring, String rcml, boolean asImsUa, String imsUaLogin, String imsUaPassword, String transferor, String transferee) {
+    public VoiceInterpreterParams(Configuration configuration, DaoManager storage, ActorRef callManager, ActorRef conferences, ActorRef bridgeManager, ActorRef smsService, Sid account, Sid phone, String version, URI url, String method, URI fallbackUrl, String fallbackMethod, URI statusCallback, String statusCallbackMethod, String referTarget, String emailAddress, ActorRef monitoring, String rcml, boolean asImsUa, String imsUaLogin, String imsUaPassword, String transferor, String transferee) {


why should we change the constructor modifier? i think there is a builder available for that,right?

Good catch! Residual modification i forgot to clean before commit.

jaimecasero · 2017-09-26T08:34:42Z

...control.jsr309/src/main/java/org/restcomm/connect/mscontrol/jsr309/Jsr309CallController.java

@@ -496,7 +496,7 @@ private void onCreateMediaSession(CreateMediaSession message, ActorRef self, Act
    }

    private void onCloseMediaSession(CloseMediaSession message, ActorRef self, ActorRef sender) throws Exception {
-        if (is(active) || is(initializing) || is(updatingMediaSession)) {
+        if (is(active) || is(initializing) || is(updatingMediaSession) || is(pending)) {


is this "pending" related to new flag "msResponsePending" ? i was expecting new flag to be here instead of this "pending"... can you clarify relation?

i see this is different actor, is msResponsePending in voiceInterpreter actor, equivalent to "pending" in jsr309CallController?

Not exactly. The boolean msResponsePending controls the sequence of communication between RC and a MS, while is(pending) verification allows the transition from pending to inactive, properly canceling a call branch. This case is particularly used when a dial fork occurs and a branch that was ringing (pending) answers back with BUSY, so we have to change to inactive. (You can see Henrique's suggestion on this here https://telestax.slack.com/archives/C0D8Y2AAG/p1481801494001255)

* New release candidate 8.2.0.1237 * New release candidate * Preliminar fix for #2271. * Patch for ticket #34390. Issue #2271. * Missing transitions from a dial fork with all branches busy, to other verbs. Issue #2271. * Inclusion of missing statusCalback for rejected calls. Issue #2271. * Preliminar patch for ticket #34415. Issue #2271. * Work in progress for ticket #34415. Issue #2271. * Patch for ticket #34415 (non optimised). Issue #2271. * Optimised patch for ticket #34415 with fix for dial action. Issue #2271. * Preparation for merge and creation of pull request. Issue #2271. * Issue #2436. Related to issues #2432, #2433 and PR #2420. * Fixing modifier. Issue #2271.

CloudBees DEV@Cloud and others added 12 commits May 17, 2017 16:27

New release candidate 8.2.0.1237

6dd7d01

New release candidate

d385ab4

Preliminar fix for #2271.

54b5d68

Patch for ticket #34390. Issue #2271.

139797b

Missing transitions from a dial fork with all branches busy, to other…

a3ebbaa

… verbs. Issue #2271.

Inclusion of missing statusCalback for rejected calls. Issue #2271.

e0a8926

Preliminar patch for ticket #34415. Issue #2271.

9a570a3

Work in progress for ticket #34415. Issue #2271.

2f14774

Patch for ticket #34415 (non optimised). Issue #2271.

ea05a6b

Optimised patch for ticket #34415 with fix for dial action. Issue #2271.

629fb03

Preparation for merge and creation of pull request. Issue #2271.

26ec2f4

Merge branch 'master' into Issue2271_CustomerWork

93b9ed7

# Conflicts: # restcomm/restcomm.interpreter/src/main/java/org/restcomm/connect/interpreter/VoiceInterpreter.java

ghjansen added Critical_Importance High-Priority labels Aug 11, 2017

gvagenas self-assigned this Aug 18, 2017

gvagenas added this to the George 8.2.0 Sprint 7 milestone Aug 18, 2017

This was referenced Aug 23, 2017

Fix to send BYE for caller when all branches are BUSY #1727

Closed

Reject notification is not sent to caller #1726

Closed

Issue #2436. Related to issues #2432, #2433 and PR #2420.

ae97269

Merge branch 'master' into Issue2271_CustomerWork

d742978

jaimecasero reviewed Sep 25, 2017

View reviewed changes

jaimecasero reviewed Sep 26, 2017

View reviewed changes

ghjansen added 2 commits September 26, 2017 07:41

Fixing modifier. Issue #2271.

32aa3cd

Merge branch 'master' into Issue2271_CustomerWork

cbea596

jaimecasero closed this Oct 3, 2017

jaimecasero deleted the Issue2271_CustomerWork branch April 11, 2018 20:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue2271 customer work #2420

Issue2271 customer work #2420

ghjansen commented Aug 11, 2017

gvagenas commented Aug 18, 2017 •

edited

Loading

ghjansen commented Aug 18, 2017

ghjansen commented Sep 6, 2017

jaimecasero Sep 25, 2017

ghjansen Sep 25, 2017

jaimecasero Sep 25, 2017

ghjansen Sep 25, 2017

jaimecasero Sep 25, 2017

ghjansen Sep 25, 2017

jaimecasero Sep 25, 2017

ghjansen Sep 25, 2017

jaimecasero Sep 26, 2017

jaimecasero Sep 25, 2017 •

edited

Loading

ghjansen Sep 25, 2017

jaimecasero Sep 25, 2017

jaimecasero Sep 25, 2017

ghjansen Sep 25, 2017

jaimecasero Sep 26, 2017

jaimecasero Sep 26, 2017 •

edited

Loading

ghjansen Sep 26, 2017

jaimecasero Sep 26, 2017

jaimecasero Sep 26, 2017

ghjansen Sep 26, 2017

Issue2271 customer work #2420

Issue2271 customer work #2420

Conversation

ghjansen commented Aug 11, 2017

gvagenas commented Aug 18, 2017 • edited Loading

ghjansen commented Aug 18, 2017

ghjansen commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaimecasero Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaimecasero Sep 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvagenas commented Aug 18, 2017 •

edited

Loading

jaimecasero Sep 25, 2017 •

edited

Loading

jaimecasero Sep 26, 2017 •

edited

Loading