[asterisk-dev] [Code Review] 2438: Add Stasis capability and AMI events around bridges

Thu Apr 11 12:53:45 CDT 2013

> On April 10, 2013, 4:19 p.m., Matt Jordan wrote:
> > team/group/bridge_construction/include/asterisk/manager.h, line 89
> > <https://reviewboard.asterisk.org/r/2438/diff/2/?file=35723#file35723line89>
> >
> >     I'm not sure we need a new AMI type.
> >     
> >     We don't have a "channel" AMI class authorization, nor do we have a type for other first class objects in Stasis. I'm not sure it's useful to promote them as such either - when using AMI, when would you want to know events about bridges but not channels?
> >     
> >     It feels like these events are better served by being part of the traditional "call" AMI class authorization. That fits within the current model of AMI; it is where the existing AMI Bridge events (useless though they are) exist; and it doesn't require people to modify their configs or expand permissions to get the bridge events.
> >     
> >     What's more, there isn't a lot of benefit to letting people filter out another class authorization. The concept of 'tags', as outlined for AMI 1.4, provides a better pub/sub mechanism then trying to set up additional filters at such a coarse granularity.

Reverted to EVENT_FLAG_CALL.

> On April 10, 2013, 4:19 p.m., Matt Jordan wrote:
> > team/group/bridge_construction/include/asterisk/stasis_bridging.h, lines 69-70
> > <https://reviewboard.asterisk.org/r/2438/diff/2/?file=35724#file35724line69>
> >
> >     The information that this flag conveys - that is, this bridge is going away - should be naturally relayed to consumers by the act of the bridge being removed from the cache. I'm not sure why we would need this property.

Removed.

> On April 10, 2013, 4:19 p.m., Matt Jordan wrote:
> > team/group/bridge_construction/include/asterisk/stasis_bridging.h, lines 62-68
> > <https://reviewboard.asterisk.org/r/2438/diff/2/?file=35724#file35724line62>
> >
> >     When would something care that these mixing values were changed?
> >     
> >     Again, only softmix currently uses these values. If we need to convey them, that feels appropriate as a JSON blob at best, although I'm not sure I know what the use case for these values is.

Removed.

> On April 10, 2013, 4:19 p.m., Matt Jordan wrote:
> > team/group/bridge_construction/include/asterisk/stasis_bridging.h, lines 55-61
> > <https://reviewboard.asterisk.org/r/2438/diff/2/?file=35724#file35724line55>
> >
> >     When is any external entity going to care that an internal flag on a bridge that prevents merges temporarily is set?

Removed.

> On April 10, 2013, 4:19 p.m., Matt Jordan wrote:
> > team/group/bridge_construction/main/bridging.c, lines 3727-3741
> > <https://reviewboard.asterisk.org/r/2438/diff/2/?file=35727#file35727line3727>
> >
> >     We don't send events for this now. Why would anything care that the internal sample rate/mixing interval was changed on a bridge?

Removed along with the flags on the snapshot.

> On April 10, 2013, 4:19 p.m., Matt Jordan wrote:
> > team/group/bridge_construction/main/stasis_bridging.c, lines 149-154
> > <https://reviewboard.asterisk.org/r/2438/diff/2/?file=35731#file35731line149>
> >
> >     Blobs

Fixed.

> On April 10, 2013, 4:19 p.m., Matt Jordan wrote:
> > team/group/bridge_construction/main/stasis_bridging.c, lines 208-213
> > <https://reviewboard.asterisk.org/r/2438/diff/2/?file=35731#file35731line208>
> >
> >     Blobs

Double fixed.

> On April 10, 2013, 4:19 p.m., Matt Jordan wrote:
> > team/group/bridge_construction/main/manager_bridging.c, line 198
> > <https://reviewboard.asterisk.org/r/2438/diff/2/?file=35729#file35729line198>
> >
> >     Rename this function to denote that it is a callback.
> >     
> >     Again, doxygen helps with these, as it will inform the reader what the purpose of the callback is.

Removed as part of refactoring into bridge blobs.

> On April 10, 2013, 4:19 p.m., Matt Jordan wrote:
> > team/group/bridge_construction/include/asterisk/stasis_bridging.h, line 130
> > <https://reviewboard.asterisk.org/r/2438/diff/2/?file=35724#file35724line130>
> >
> >     Doxygen comment the actual struct as well.

Done.

> On April 10, 2013, 4:19 p.m., Matt Jordan wrote:
> > team/group/bridge_construction/main/manager_bridging.c, lines 230-234
> > <https://reviewboard.asterisk.org/r/2438/diff/2/?file=35729#file35729line230>
> >
> >     I can't think of a situation in which a channel would both leave and enter the bridge at the same time. That should be two different snapshot updates.
> >     
> >     You should be able to return immediately if a channel entered.

> On April 10, 2013, 4:19 p.m., Matt Jordan wrote:
> > team/group/bridge_construction/main/manager_bridging.c, lines 248-264
> > <https://reviewboard.asterisk.org/r/2438/diff/2/?file=35729#file35729line248>
> >
> >     While there are test suite events for these notifications, they aren't really a current part of the set of events that AMI sends out, and I'm not sure we have to absolutely have them.
> >     
> >     One of the things that I don't like about this currently is that these primarily make sense only in multi party bridges, but we have them as part of the bridging snapshot - that is, an object that should contain properties that all bridges have instead has specific bridging technology data in it. If we are going to have these, I'd prefer them to either by JSON added by softmix.

Video events have been removed for now.

> On April 10, 2013, 4:19 p.m., Matt Jordan wrote:
> > team/group/bridge_construction/main/manager_bridging.c, lines 214-246
> > <https://reviewboard.asterisk.org/r/2438/diff/2/?file=35729#file35729line214>
> >
> >     I'm not entirely sure I like how this is being handled. While having a container of channel names to track the channels on the bridge snapshot is somewhat convenient, it feels like we're purposefully losing information only to reconstruct it again later.
> >     
> >     We know when a channel leaves/enters a bridge both what channel left/entered as well as what bridge it was. Why not have a specific Stasis message type for that which contains both the channel snapshot and the bridge snapshot, along with a JSON blob that states what just happened?
> >     
> >     As it is, this is an O(n^2) operation, which feels incredibly costly when we had the information at one point in time.

Bridge blobs added with enter and leave events refactored on top of that.

> On April 10, 2013, 4:19 p.m., Matt Jordan wrote:
> > team/group/bridge_construction/include/asterisk/stasis_bridging.h, lines 42-48
> > <https://reviewboard.asterisk.org/r/2438/diff/2/?file=35724#file35724line42>
> >
> >     Video information should not be properties on the bridge snapshot.
> >     
> >     1) Not all bridges will support video modes or changes in source. In fact, only softmix really supports these concepts. (And things like the holding bridge will never really need them)
> >     
> >     2) This information, which is a relatively rare kind of event, is probably better served by a JSON blob accompanying a bridge snapshot.

Removed.

- opticron

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

On April 10, 2013, 2:15 p.m., opticron wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2438/
> -----------------------------------------------------------
> 
> (Updated April 10, 2013, 2:15 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-21057
>     https://issues.asterisk.org/jira/browse/ASTERISK-21057
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This adds the base of Stasis messages, topics, message types, caches, and snapshot generation points within the bridging code to facilitate access to bridge information without accessing bridges directly and also adds AMI events for bridge changes built on top of that Stasis layer.
> 
> The new function in strings.h (ast_str_container_alloc) allocates a new AO2 container for bare strings since this functionality seems to be required quite a bit more often lately.
> 
> 
> Diffs
> -----
> 
>   team/group/bridge_construction/include/asterisk/manager.h 385233 
>   team/group/bridge_construction/include/asterisk/stasis_bridging.h PRE-CREATION 
>   team/group/bridge_construction/include/asterisk/strings.h 385233 
>   team/group/bridge_construction/main/asterisk.c 385233 
>   team/group/bridge_construction/main/bridging.c 385233 
>   team/group/bridge_construction/main/manager.c 385233 
>   team/group/bridge_construction/main/manager_bridging.c PRE-CREATION 
>   team/group/bridge_construction/main/manager_channels.c 385233 
>   team/group/bridge_construction/main/stasis_bridging.c PRE-CREATION 
>   team/group/bridge_construction/main/strings.c 385233 
> 
> Diff: https://reviewboard.asterisk.org/r/2438/diff/
> 
> 
> Testing
> -------
> 
> Tested with Confbridge and BridgeWait to verify correct events are generated when calls enter and leave bridges.
> 
> 
> Thanks,
> 
> opticron
> 
>

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