[asterisk-dev] [Code Review] 2383: Refactor Dialing to publish Stasis-Core messages

[Matt Jordan]

> On March 31, 2013, 2:48 a.m., opticron wrote:
> > /trunk/main/stasis_channels.c, lines 41-57
> > <https://reviewboard.asterisk.org/r/2383/diff/5/?file=35128#file35128line41>
> >
> >     Is there any reason for adding all these '__'?
> 
> Matt Jordan wrote:
>     They were there before?
>     
>     I'd prefer it if we didn't prefix them with '__' myself - but I'm open to other opinions.
> 
> opticron wrote:
>     Actually, I yanked them from trunk when I fixed all the message type accessors.  The ones you have here are probably left over merge cruft.

Cool, renamed


> On March 31, 2013, 2:48 a.m., opticron wrote:
> > /trunk/main/stasis_channels.c, lines 493-494
> > <https://reviewboard.asterisk.org/r/2383/diff/5/?file=35128#file35128line493>
> >
> >     Are message types using multi channel blobs supposed to make their own message type or are they supposed to use the type from ast_multi_channel_blob_type()?  Should ast_channel_blob messages be defining their own types as well?
> 
> Matt Jordan wrote:
>     There are pros and cons to each approach.
>     
>     If an ast_multi_channel_blob (or an ast_channel_blob) is simply a payload for a message type, then the message router becomes more useful - you simply define a callback for each message type and you 'know' that the payload you're delivered contains the data you want. This makes the JSON extraction simpler. On the other hand, this also means that you will inevitably have a larger set of stasis message types, and defining those becomes a tad trickier, particularly for modules that may not be present.
>     
>     If, however, an ast_multi_channel_blob is instead treated as its own message type, then each consumer will have to extract the type from the JSON to determine if they need to handle that particular message. This, once again, creates message routers in each consumer. I'm not a fan of that: it increases consumer complexity as well as creates a significant amount of duplicate 'boiler-plate' logic. On the other hand, it does keep the stasis message types lower, and reduces the number of header files that have to be included.
>     
>     Right now, nothing is really using the ast_multi_channel_blob_type, and it could be removed. If we decide to go with the blob == type approach, then the dial type can be removed.
> 
> opticron wrote:
>     +1 for having more message types and making the message router more useful.  If we go down this road, does it make sense to remove the mandatory type fields from the json blobs and their associated accessor functions?

I'm not sure yet. I think the multi-channel blobs are going to be used more often in conjunction with other objects, so it felt appropriate to not require them to be their own message type. I think we need to just see where the pain points are in the producers/consumers. If we find ourselves writing a lot of type extraction code, then we may want to go with explicit message types.


- Matt


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


On March 29, 2013, 10:33 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2383/
> -----------------------------------------------------------
> 
> (Updated March 29, 2013, 10:33 p.m.)
> 
> 
> Review request for Asterisk Developers and David Lee.
> 
> 
> Bugs: ASTERISK-21196
>     https://issues.asterisk.org/jira/browse/ASTERISK-21196
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> As part of the CDR work for Asterisk 12 (see https://wiki.asterisk.org/wiki/display/AST/Asterisk+12+CDR+Specification), we need Dial information published over Stasis-Core. This patch refactors app_dial to publish the necessary events.
> 
> The Dial based events are slightly different in Asterisk 12 than in other previous versions:
>  * Dial is now two events, DialBegin and DialEnd. This matches the nomenclature of other AMI events. See https://wiki.asterisk.org/wiki/display/AST/AMI+1.4+Specification for more information.
>  * Dial events now occur at the beginning of a dial operation and when the status of the dial operation is known. Previously, it occurred on application exit, which occurred after bridging.
> 
> Note that other applications will need refactoring as well (such as the Dial Framework, Queue, FollowMe, etc.) - however, in order to limit the scope of the work, I've kept it only to app_dial at this point.
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_dial.c 384376 
>   /trunk/apps/app_userevent.c 384376 
>   /trunk/include/asterisk/channel.h 384376 
>   /trunk/include/asterisk/stasis_channels.h PRE-CREATION 
>   /trunk/main/channel.c 384376 
>   /trunk/main/channel_internal_api.c 384376 
>   /trunk/main/dial.c 384376 
>   /trunk/main/features.c 384376 
>   /trunk/main/manager_channels.c 384376 
>   /trunk/main/pbx.c 384376 
>   /trunk/main/stasis_channels.c PRE-CREATION 
>   /trunk/pbx/pbx_realtime.c 384376 
>   /trunk/tests/test_stasis_channels.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/2383/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130331/4d28959a/attachment.htm>