[asterisk-dev] Change in testsuite[master]: stasis: set a channel variable on websocket disconnect error

[Matt Jordan (Code Review)]

Matt Jordan has posted comments on this change.

Change subject: stasis: set a channel variable on websocket disconnect error
......................................................................


Patch Set 2:

(5 comments)

https://gerrit.asterisk.org/#/c/18/2/tests/rest_api/applications/stasisstatus/ari_client.py
File tests/rest_api/applications/stasisstatus/ari_client.py:

Line 71:         while not self.clean:
       :             LOGGER.debug('{0} I\'m not so fresh so clean.'.format(self))
       :             time.sleep(1)
There's a few problems with this particular approach.

First, using any kind of sleep mechanism generally plays poorly with twisted (Google 'twisted sleep' and you'll get quite a few hits). While you are sleeping, you have actually blocked the twisted reactor - which means you're preventing a whole lot of things from getting done, including - potentially - the value of clean from ever changing.

You really have two options here:
1. Have connect_websocket return False if it isn't clean, or else have the calling code handle that. The calling code should then handle the problem by calling reactor.callLater and attempting again. I prefer this mechanism personally, as it is up to the caller to know what to do if they can't establish a websocket.

2. Create a callLater reactor loop here. While this would work, you will also need to implement a retry count. Consider the following:
1) Connect the WebSocket
2) Originate something and stick it in Echo
3) Call this method again

This will get stuck, as the channel count will never be 0. The caller is the one who is aware of the origination - as such, they're probably better off handling any retries.


Line 270:         resource              --  The resource to use to construct the endpoint
        :                                   for the ARI request. (optional)
        :                                   (default None.) If no value is provided,
        :                                   resource will automatically be generated in
        :                                   the form: 'LOCAL/<app_name>@Acme'.
To make this more generically useful, I'd have the caller pass in the endpoint, and not the resource.

Right now, this approach constrains the caller to follow the dialplan convention of having a context named "Acme", as well as having to use a Local channel. That's an implementation detail of the user of this object. The AriClient should manage the origination, and not where the origination goes to.


https://gerrit.asterisk.org/#/c/18/2/tests/rest_api/applications/stasisstatus/monitor.py
File tests/rest_api/applications/stasisstatus/monitor.py:

Line 45:         self.__ami.registerEvent('VarSet', self.__on_ami_varset)
       :         self.__ami.registerEvent('UserEvent', self.__on_ami_user_event)
       :         self.__ari_client.register_observers('on_stasisend',
       :                                              self.__on_stasisend)
       :         self.__ari_client.register_observers('on_stasisstart',
       :                                              self.__on_stasisstart)
       :         self.__ari_client.register_observers('on_channelvarset',
       :                                              self.__on_channelvarset)
I'm not sure you need both AMI and ARI in this class.

Every channel variable change will be reflected through AMI, regardless of the location of the channel.

In ARI, however, you'll only get notified of the changes if you are subscribed to the channel.

As it is, in this class, we'll get the channel variable changes twice, and update the channel value twice - which feels like extra work we don't need.

If the purpose is to track the channel variables that are changed by a channel controlled through ARI, then I'd just use the ARI object. If you need knowledge of a channel while it is in the dialplan, then ARI can subscribe to the channel.

If, however, you don't want to muck around with ARI subscriptions or you really want to know all variable changes on a channel at all times, then I'd just use AMI.


https://gerrit.asterisk.org/#/c/18/2/tests/rest_api/applications/stasisstatus/test-config.yaml
File tests/rest_api/applications/stasisstatus/test-config.yaml:

Line 31:         - ARI
       :         - Stasis
Stasis is, in many ways, synonymous currently with ARI. Since we already do have an ARI tag, I'd just use that one.


Line 33:         - Application
       :         - STASISSTATUS
I'd remove these two tags.

Tags are generally introduced when they match a subset of tests that, together, test one largely granular feature. Unless we think we're going to have a dozen or so tests that exercise a particular feature, I wouldn't add another tag that the Test Suite knows about.

(Admittedly, we have some silly tags that crept in :-)

If you want to see what tags exist already, runtests.py -L will dump them out.


-- 
To view, visit https://gerrit.asterisk.org/18
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f7dadfd429bd30e9f07a531f47884d8c923fc13
Gerrit-PatchSet: 2
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes