RC Client object updated with pushClientIdentity in terms of RPNS activity by agafox · Pull Request #2412 · RestComm/Restcomm-Connect

agafox · 2017-08-07T14:29:04Z

No description provided.

gvagenas

@agafox I believe there is no need for any changes in the API or the DB schema

gvagenas · 2017-08-08T09:37:56Z

@@ -0,0 +1,48 @@
+#SQL Script for MySQL/MariaDB to update DB with the schema changes


@agafox The DB update script needs to be moved to the private repository together with the other DB update scripts

I thought that we provide update scripts if DB changes. btw, @deruelle said that all changes for this feature in RC we should make in public repository.

@agafox you are right this new feature should be in the public repository. The DB schema upgrade script for existing customer deployments should be in the private repository

Removed script from public repository

gvagenas · 2017-08-08T09:48:04Z

 voice_application_sid VARCHAR(34),
-uri MEDIUMTEXT NOT NULL
+uri MEDIUMTEXT NOT NULL,
+push_client_identity VARCHAR(34)


@agafox the Push Client Identity is the Client SID so there is no need to add an extra column to repeat the same data.

First of all we need to add flag to DB that this RC Client uses mobile application. This flag defines how CallManager will handle SIP Invite. After a long discussion we decided to use Identity instead of just a flag.

@agafox I believe this is not the proper approach and this approach will not be enough to cover all use cases. Keep in mind I don't have 100% clear picture of the feature.

Let me elaborate on my thoughts so we can discuss here.

Let's say we have client bob.

Bob registers from 2 sip clients at the same time:

Cisco desk phone (or any other softphone on laptop etc that doesn't need push notification)

Android Olympus application (that needs push notification)

Because Restcomm keeps two active registrations for client bob, whenever Restcomm needs to reach to bob it will Dial to the two AOR according to the registrations table.

As long as the mobile device is active and Android Olympus responds to Option requests from Restcomm, the dial fork will work fine.

When the mobile device goes to sleep and no longer responds to Option requests, Restcomm will remove the Android registration and will keep only the registration for the Cisco desk phone. So when someone calls bob, only the Cisco phone will get INVITE.

Even if CallManager sees that this client needs to send a Push Notification (checking the new table column), the AOR in the DB will be of the Cisco device only and will not have the Android AOR.

Even if we don't remove the mobile client's Registration on timeout (bad idea - just for example) and we just provide the Client SID in the DB column, or any other flag indicating that the client needs Push Notification, Restcomm won't be able to tell which of the 2 registrations needs Push Notification and will send notification to both Cisco and Android.

Modifying the Client model to support Push Notification is bad idea because in order to work properly we will have to give up the Dial Forking that now we support.

Currently, Dial Bob will dial to ANY AOR in the DB for Bob.

With the extra property indicating the need for Push Notification, we need different client for bob_deskphone, and another one for mobile bob_android.

I believe the proper way will be to implement this feature at the Registrar, which should check the User-Agent header and if the sip client is a mobile application should store the AOR for the Push Notification service. We should have a new table for this.

gvagenas · 2017-08-10T08:12:29Z

@agafox your work up to now is fine. Please proceed to the rest of the patch and ping me to review the rest of the work

gvagenas

@agafox The Push Notification should be sent only after the Extensions allowed this call to happen.

gvagenas · 2017-08-16T08:55:06Z

-                ExtensionController ec = ExtensionController.getInstance();
-                IExtensionCreateCallRequest er = new CreateCall(fromUser, toUser, "", "", false, 0, CreateCallType.CLIENT, client.getAccountSid(), null,null, null, null);
-                ec.executePreOutboundAction(er, this.extensions);
+                long delay = sendPushNotificationIfNeeded(toClient.getPushClientIdentity());


@agafox The Push Notification request should be sent AFTER the extension has been executed and when the call is allowed, that is after if (er.isAllowed()) {

gvagenas · 2017-08-16T08:57:17Z

-                    resp.send();
-                }
-                ec.executePostOutboundAction(er, this.extensions);
+                }, system.dispatcher());


@agafox since the Push Notification is a task not related to Akka actors etc, maybe we should create new Execution Context or use the one created for the S3 upload, and use it here to execute this task.

I'm not sure here. CallManager contains objects like 'createCallRequest' and to handle this case proper we need to provide akka actor execution(single threaded) contract. Dispatcher I've used provides this contract for sure, but any other executor will bring us to multi-threaded mess.

@agafox, system.dispatcher is used for all Akka operations. If for any reason the Push Notification operation blocks or takes a long time to complete, this task will block the threads that should be used for Akka operations only.
This happened when we used the Akka thread pool to upload the Recordings to Amazon S3. For a reason the upload was taking a long time and we end up to a thread starvation and thus no thread was available for Akka actors to take a new call for example.
For this reason, we provided a new Execution Context:

Restcomm-Connect/restcomm/restcomm.application/src/main/resources/application.conf

Line 34 in 2bb19ef

restcomm-blocking-dispatcher {

. Check here how we use it:

Restcomm-Connect/restcomm/restcomm.dao/src/main/java/org/restcomm/connect/dao/mybatis/MybatisRecordingsDao.java

Line 83 in 2bb19ef

Future<Boolean> f = Futures.future(new Callable<Boolean>() {

gvagenas · 2017-08-16T08:59:38Z

+                ClientsDao clients = storage.getClientsDao();
+                Client client = clients.getClient(request.to().replaceFirst("client:", ""));
+                if (client != null) {
+                    long delay = sendPushNotificationIfNeeded(client.getPushClientIdentity());


@agafox Similar to the P2P calls, the Push Notification request should be sent AFTER the extension has been executed and when the call is allowed, that is after if (request.isAllowed()) {

gvagenas · 2017-08-16T09:00:39Z

+                                throw new RuntimeException(e);
+                            }
+                        }
+                    }, system.dispatcher());


@agafox same as before, since the Push Notification is a task not related to Akka actors etc, maybe we should create new Execution Context or use the one created for the S3 upload, and use it here to execute this task.

Same thing here. I think in terms of actor execution contract it must be an actor dispatcher. BTW what is the problem here?

@agafox see my previous answer and check here http://doc.akka.io/docs/akka/2.5.3/scala/dispatchers.html#blocking-needs-careful-management if you need more details on the problem

…ation server task

gvagenas · 2017-08-18T10:51:08Z

Great work @agafox . I will proceed to merge to master

agafox added 3 commits August 7, 2017 16:36

Model + MyBatis update

9fa7b4e

SQL update

62e2bb6

update script for MySQL/MariaDB

b339364

agafox requested a review from gvagenas August 7, 2017 14:29

otsakir added the Peer Review label Aug 7, 2017

agafox added 4 commits August 8, 2017 09:37

missed sql update

6cfb5d8

missed sql update

82a5665

ClientsDaoTest updated

10e68a7

ClientConverter updated

a1e4f93

gvagenas suggested changes Aug 8, 2017

View reviewed changes

update script removed from public repository

7cdf5ca

gvagenas self-assigned this Aug 10, 2017

gvagenas added the unplanned label Aug 10, 2017

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

agafox and others added 3 commits August 14, 2017 14:28

Push Notification Server support added

4719cde

PushNotificationServerTest bugfix

a018c11

sip-servlets version update

8d3ca13

gvagenas suggested changes Aug 16, 2017

View reviewed changes

agafox added 3 commits August 16, 2017 12:59

ExtensionController with executePreOutboundAction before push notific…

c2de08e

…ation server task

Push server HTTP request in a separate thread with blocking dispatcher

b2f2f00

Merge branch 'master' into push-notification-server

dad461f

gvagenas merged commit dad461f into master Aug 18, 2017

gvagenas removed the Peer Review label Aug 18, 2017

jaimecasero deleted the push-notification-server branch November 22, 2017 09:40

jaimecasero restored the push-notification-server branch November 22, 2017 09:41

jaimecasero deleted the push-notification-server branch November 22, 2017 09:41

jaimecasero restored the push-notification-server branch November 22, 2017 09:41

jaimecasero deleted the push-notification-server branch November 22, 2017 09:41

		@@ -0,0 +1,48 @@
		#SQL Script for MySQL/MariaDB to update DB with the schema changes

Conversation

agafox commented Aug 7, 2017

Uh oh!

gvagenas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gvagenas commented Aug 10, 2017

Uh oh!

gvagenas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gvagenas commented Aug 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants