[asterisk-dev] [Code Review]: Move Newexten and UserEvent AMI events to Stasis; adding JSON messages to stasis-core

[opticron]

> On March 14, 2013, 11:52 a.m., opticron wrote:
> > /trunk/main/manager_channels.c, line 95
> > <https://reviewboard.asterisk.org/r/2381/diff/3/?file=34483#file34483line95>
> >
> >     This should probably use manager_build_channel_state_string since it's a message being generated from a channel snapshot and it should be processed like the other channel state AMI updates.
> 
> David Lee wrote:
>     Newexten has 'Extension' instead of 'Exten'. Is it really worth making that change?
>     
>     Newexten also has 'Application' and 'AppData' fields, which aren't currently in the channel_state_string. Should we add it for all events that use that function, or just optionally for Newexten?

For Application and AppData, that will depend on whether we consider them critical channel information.  If not, it can use the same mechanism as varset and userevent to add non-standard headers to the channel state event.

As for Exten vs Extension, the standard channel event information needs to be consistent across channel state AMI events and I'd go with Extension since there isn't similar abbreviation elsewhere in the message.  If we want to be slightly more backwards compatible, the Newchannel event would need an additional/redundant Exten header, but I'd prefer to change it and document that it was changed.


On March 14, 2013, 11:52 a.m., David Lee wrote:
> > I'll also make the argument that varset and userevent manager events should use manager_build_channel_state_string and the generic mechanism for generating these events since they pull data from a channel state snapshot.
> 
> David Lee wrote:
>     Varset is only using the uniqueid and channel name. Userevent is only using the uniqueid. Do we want to add the rest of the channel state to those events?

I'd say yes since this is channel-related and channel-related events should have at least the base set of channel information.


- opticron


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2381/#review8047
-----------------------------------------------------------


On March 14, 2013, 11:07 a.m., David Lee wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2381/
> -----------------------------------------------------------
> 
> (Updated March 14, 2013, 11:07 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch started out simply as fixing the bouncing tests introduced
> in r382685, but required some other changes to give it a decent
> implementation.
> 
> To fix the bouncing tests, the UserEvent and Newexten AMI events
> needed to be refactored to dispatch via Stasis. Dispatching directly
> to AMI resulted in those events sometimes getting ahead of the
> associated Newchannel events, which would understandably confuse anyone.
> 
> I found that instead of creating a zillion different message types and
> structures associated with them, it would be preferable to define a
> message type that has a channel snapshot and a blob of structured data
> with a small bit of additional information. The JSON object model
> provides a very nice way of representing structured data, so I went
> with that.
> 
>  * Move JSON support from res_json.c to main/json.c
>    * Made libjansson-dev a required dependency
>  * Added an ast_channel_blob message type, which has a channel
>    snapshot and JSON blob of data.
>  * Changed UserEvent and Newexten events so that they are dispatched
>    via ast_channel_blob messages on the channel's topic.
>  * Got rid of the ast_channel_varset message; used ast_channel_blob
>    instead.
>  * Extracted the manager functions converting Stasis channel events to
>    AMI events into manager_channel.c.
> 
> 
> Diffs
> -----
> 
>   /trunk/main/json.c PRE-CREATION 
>   /trunk/main/manager.c 382826 
>   /trunk/main/channel.c 382826 
>   /trunk/configure.ac 382826 
>   /trunk/include/asterisk/autoconfig.h.in 382826 
>   /trunk/include/asterisk/channel.h 382826 
>   /trunk/include/asterisk/manager.h 382826 
>   /trunk/configure 382826 
>   /trunk/apps/app_userevent.c 382826 
>   /trunk/main/manager_channels.c PRE-CREATION 
>   /trunk/main/pbx.c 382826 
>   /trunk/pbx/pbx_realtime.c 382826 
>   /trunk/res/res_json.c 382826 
>   /trunk/res/res_json.exports.in 382826 
>   /trunk/tests/test_json.c 382826 
> 
> Diff: https://reviewboard.asterisk.org/r/2381/diff
> 
> 
> Testing
> -------
> 
> Ran bouncing testsuite tests 10 times without failure.
> 
> 
> Thanks,
> 
> David
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130315/da20ec4d/attachment-0001.htm>