[asterisk-dev] [Code Review] 3070: bridges: Add two new properties to bridges and bridge snapshots - the name of a creator and the name the creator uses to refer to that bridge
Jonathan Rose
reviewboard at asterisk.org
Mon Dec 16 13:48:19 CST 2013
> On Dec. 14, 2013, 1:39 p.m., Paul Belanger wrote:
> > /branches/12/res/ari/ari_model_validators.h, lines 1066-1067
> > <https://reviewboard.asterisk.org/r/3070/diff/3/?file=49603#file49603line1066>
> >
> > any change on dropping the bridge_ prefix. Seems redundant this is a bridge model.
> >
> > For example, we have id not bridge_id, same with channel, not bridge_channels.
> >
> > Sadly, bridge_type, and bridge_class existing already.
> >
> > -1 for inconsistent variable names
>
> Jonathan Rose wrote:
> I feel that this fits in better with bridge_type and bridge_class conceptually. With channels being named as channels instead of bridge_channels, it makes sense to deviate from that because channels are another resource. And id... well, that's just always ID.
>
> I don't know if I'd really describe it as being inconsistent. I'll let dlee or kmoore chime in if they are able though.
>
> Paul Belanger wrote:
> Defiantly inconsistent, just look at the channel model. It is not channel_name, it is name. Same for CallerID, it is not callerid_name. Events is the red headed step child, it is eventname. So you can see, if we move forward with this, we'll have 3 different way to parse the name variable in our models.
>
> Additionally, we have changed the format of the variables again. We seem to be back to snake_case, I believe the decision was to keep everything camelCase[1].
>
> [1] http://lists.digium.com/pipermail/asterisk-dev/2013-October/063003.html
Well, at the end of the day this isn't really something I feel strongly about, so I'll go ahead and just drop the 'bridge_' from the variable names that I'm adding. You might consider filing an issue against the other misnamed variables, but I personally have no interest in changing them as part of this review.
> On Dec. 14, 2013, 1:39 p.m., Paul Belanger wrote:
> > /branches/12/rest-api/api-docs/bridges.json, lines 505-514
> > <https://reviewboard.asterisk.org/r/3070/diff/3/?file=49607#file49607line505>
> >
> > Why are these required, but every place else appear to be optional?
>
> Jonathan Rose wrote:
> They are guaranteed to at least appear in the output even if they are blank. I don't really know how appropriate that is. It might be better to have them be '<none>' like with the manager events... I'm not sure, so I'll wait for dlee or kmoore's opinion on this one as well.
>
> Paul Belanger wrote:
> If the parameter is not used, it should have a return value as Null, not a string with the value of '<none>'. This will be a nightmare to parse.
After experimenting with it for a while, I think it's best to just leave these as blank strings when they are unnamed. The json packing and model validation don't seem to play too nicely with NULL values for strings. I don't think having a zero length string instead of NULL makes a huge difference.
- Jonathan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3070/#review10418
-----------------------------------------------------------
On Dec. 13, 2013, 3:40 p.m., Jonathan Rose wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3070/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2013, 3:40 p.m.)
>
>
> Review request for Asterisk Developers, David Lee, kmoore, Mark Michelson, and rmudgett.
>
>
> Repository: Asterisk
>
>
> Description
> -------
>
> So there has been at least a little bit of clammoring internally about having the need to be able to tell when a bridge belongs to a specific parking lot or app_confbridge conference. This patch provides two new properties to the bridge.
>
> BridgeCreator - Provides the name of a system which is responsible for creating the bridge. This includes things such as 'AgentPool', 'Parking', 'ConfBridge', 'Stasis', and other systems which can create bridges.
>
> BridgeName - Provides the name given to the bridge to refer to it internally by the creator.
>
> BridgeCreator may be set or it may be unset (in which case it will appear as a zero length string). BridgeName will only appear when BridgeCreator is also set, but it is also optional. So you have the following possibilities:
>
> neither BridgeCreator nor BridgeName
> BridgeCreator but not BridgeName
> BridgeCreator and BridgeName
>
>
> Diffs
> -----
>
> /branches/12/rest-api/api-docs/bridges.json 403725
> /branches/12/res/res_stasis.c 403725
> /branches/12/res/parking/parking_bridge.c 403725
> /branches/12/res/ari/ari_model_validators.c 403725
> /branches/12/res/ari/ari_model_validators.h 403725
> /branches/12/main/stasis_bridges.c 403725
> /branches/12/main/manager_bridges.c 403725
> /branches/12/main/bridge_basic.c 403725
> /branches/12/main/bridge.c 403725
> /branches/12/include/asterisk/stasis_bridges.h 403725
> /branches/12/include/asterisk/bridge_internal.h 403725
> /branches/12/include/asterisk/bridge.h 403725
> /branches/12/doc/appdocsxml.xslt 403725
> /branches/12/apps/app_confbridge.c 403725
> /branches/12/apps/app_bridgewait.c 403725
> /branches/12/apps/app_agent_pool.c 403725
>
> Diff: https://reviewboard.asterisk.org/r/3070/diff/
>
>
> Testing
> -------
>
> Checked output for Manager events displaying Bridge snapshots
>
> Event: BridgeEnter
> Privilege: call,all
> BridgeUniqueid: a72735f6-04ac-499b-a2da-bb997d6f99ab
> BridgeType: parking
> BridgeTechnology: holding_bridge
> BridgeCreator: Parking
> BridgeName: default
> BridgeNumChannels: 1
> Channel: PJSIP/pjgold-00000000
> ChannelState: 6
> ChannelStateDesc: Up
> CallerIDNum: pjgold
> CallerIDName: pjgold
> ConnectedLineNum: <unknown>
> ConnectedLineName: <unknown>
> AccountCode:
> Context: default
> Exten: 700
> Priority: 1
> Uniqueid: 1386949677.0
>
>
> Checked output of GET /bridges on ARI in petstore:
>
> [
> {
> "id": "a72735f6-04ac-499b-a2da-bb997d6f99ab",
> "channels": [],
> "technology": "holding_bridge",
> "bridge_creator": "Parking",
> "bridge_class": "parking",
> "bridge_type": "holding",
> "bridge_name": "default"
> },
> {
> "id": "80e19b8f-5a04-464a-885c-4c119764bf83",
> "channels": [
> "1386949828.4"
> ],
> "technology": "holding_bridge",
> "bridge_creator": "BridgeWait",
> "bridge_class": "base",
> "bridge_type": "holding",
> "bridge_name": "testname"
> }
> ]
>
>
> Thanks,
>
> Jonathan Rose
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20131216/770f65b7/attachment-0001.html>
More information about the asterisk-dev
mailing list