Push notification server support for text messages #2548
Conversation
…while implementing SIP Message push notifications
|
Link to test-suite results: https://cxs.restcomm.com/view/Restcomm/job/UPS_RestComm-Specific-Branch/6/#showFailuresLink There is no regression at all |
| } | ||
| system.scheduler().scheduleOnce(Duration.create(delay, TimeUnit.MILLISECONDS), new Runnable() { |
There was a problem hiding this comment.
this task logic is mostly sipservlet API related, this would be better handled in SIPServlet thread pool by using https://mobicents.ci.cloudbees.com/job/RestcommSipServlets-Release/lastSuccessfulBuild/artifact/documentation/jsr289-extensions-apidocs/org/mobicents/javax/servlet/sip/SipSessionsUtilExt.html#scheduleAsynchronousWork(java.lang.String,%20org.mobicents.javax.servlet.sip.SipApplicationSessionAsynchronousWork)
There was a problem hiding this comment.
The main motivation here is to use akka ecosystem. The same thing was implemented in CallManager and we agreed with @gvagenas about that
There was a problem hiding this comment.
well, this is new dicussion with @gvagenas . There is a pending change in RC to use SIPServlets from proper SipServletsASyncWork, so no concurrent situations lead to inconsistencies, but agree this is a different topic, up to you on how to do this...
There was a problem hiding this comment.
Let's keep it with akka scheduler just to leave this functionality with the same approach in all parts of RC. After that we can make a decision to use akka ecosystem or SipServletsASyncWork and to create new task for refactoring in whole RC code
| } else { | ||
| delay = pushNotificationServerHelper.sendPushNotificationIfNeeded(toClient.getPushClientIdentity()); | ||
| } | ||
| system.scheduler().scheduleOnce(Duration.create(delay, TimeUnit.MILLISECONDS), new Runnable() { |
There was a problem hiding this comment.
| if (logger.isDebugEnabled()) { | ||
| logger.debug("Push server notification to client with identity: '" + pushClientIdentity + "' added to queue."); | ||
| } | ||
| Futures.future(new Callable<Void>() { |
There was a problem hiding this comment.
why dont you use Downloader actor? is already wrapping the http client, and later the refactorign to use acutal nonBlocking http client will be easier if we all use the saem http Downloader actor...
There was a problem hiding this comment.
Downloader actor is for downloading RCMLs. I'm not sure that it's good thing to use it for internal API calls
There was a problem hiding this comment.
you are right ,there are some xml validation when response is ready, what a pity...
| wireMockRule.addMockServiceRequestListener(new RequestListener() { | ||
| @Override | ||
| public void requestReceived(com.github.tomakehurst.wiremock.http.Request request, com.github.tomakehurst.wiremock.http.Response response) { | ||
| if (request.getAbsoluteUrl().contains("/api/notifications") && response.getStatus() == 200) { |
There was a problem hiding this comment.
is this ismulating alice phone receiving push notification and register the phone through the mobile app? quite smart choice of mock usage... ;)
| @SuppressWarnings("Duplicates") | ||
| @Deployment(name = "SmsPushNotificationServerTest", testable = false) | ||
| public static WebArchive createWebArchiveNoGw() { | ||
| logger.info("Packaging Test App"); |
There was a problem hiding this comment.
call reconfigurePorts here.
use new utility and make sure all ports are mapped so we allow parallel testing as in https://github.com/RestComm/Restcomm-Connect/blob/master/restcomm/restcomm.testsuite/src/test/java/org/restcomm/connect/testsuite/sms/SmsTest.java#L577
| private static final String CLIENT_PASSWORD = "qwerty1234RT"; | ||
|
|
||
| @Rule | ||
| public WireMockRule wireMockRule = new WireMockRule(8090); |
There was a problem hiding this comment.
this port should be dyn assigned to allow test parallelization as in https://github.com/RestComm/Restcomm-Connect/blob/master/restcomm/restcomm.testsuite/src/test/java/org/restcomm/connect/testsuite/telephony/TestDialVerbPartThree.java#L75
| if (logger.isDebugEnabled()) { | ||
| logger.debug("Push server notification to client with identity: '" + pushClientIdentity + "' added to queue."); | ||
| } | ||
| Futures.future(new Callable<Void>() { |
There was a problem hiding this comment.
why dont we use the returned Future and register a OnComplete functions, instead of scheduling a fixed delay task later...? OnComplete function coudl be passed as argument to make it generic, and let client configure oncomplete logic
No description provided.