[asterisk-dev] [Code Review] 3471: Filesystem based dynamic MoH classes

[Matt Jordan]

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


While I appreciate the contribution to Asterisk and the intended purpose of this patch, at this time, I don't think this patch is appropriate for inclusion.

(1) Having custom file formats via the proposed playlists.txt is not something to encourage. Asterisk has an understood approach to defining its configuration; adding a custom schema creates a burden on system administrators as "yet another thing to understand". Even the new configuration framework/sorcery API in Asterisk 12+ still builds on the schemas defined for .conf files.

(2) By creating your own file format, you discard a substantial amount of work that has gone into the existing file reading APIs. Those APIs allow for you to check a .conf file to determine if it has changed and needs to be re-read - by rolling your own format, you are having to read the format each time Asterisk needs to determine if anything has changed in the MOH definition. While you could implement a similar mechanism, re-inventing the wheel should be a sign that something is not correct with this approach.

(3) Even assuming there was a playlists.conf, I don't understand how there is a substantial benefit of having a playlists file over files in a directory. The files in the directory can already be re-scanned for new files. The files don't even have to exist in that directory: symbolic links allow for a user to have the actual files located in some other location on the server. This feels like a lot of extra work for very little benefit.

Now, looking at the actual use case quoted on the issue:

"One of the things I needed was the ability to set the music on hold class for a call based on information gathered at the time of the call."

That's fine: but how does the CHANNEL function's musicclass attribute (https://wiki.asterisk.org/wiki/display/AST/Asterisk+11+Function_CHANNEL) not already allow for this?

"This patch allows us to build a MOH class and playlists "on the fly" that are then active when the caller puts calls on hold or if the AGI/AMI needs to play back MOH. Without this patch all combinations of music files and all MOH classes would have to be defined in the configuration file or database before Asterisk is started. In theory you could modify configuration "on the fly" however if I recall a reload of MOH kills other calls on hold or other nasty things happened."

(1) The approach taken here gains a small amount of dynamic ability - that can mostly be captured by existing mechanisms - at a performance and maintenance cost. That's not acceptable.
(2) The goal of having musiconhold 'discover' music classes at run-time is laudable, but is also possible with frameworks in Asterisk 12/trunk today. This doesn't require playlists files, a defined directory structure, or other non-standard approaches. If musiconhold was made to use sorcery, two things would be possible:
   (a) It would - when querying a realtime backend - grab the requested class if it existed or fail if the class did not. This would not require a reload of the module when adding a new class.
   (b) If using a static backend (such as a conf file), a reload would still be necessary. However, since sorcery guarantees thread safety and that existing operations in flight continue on without being affected, this would not result in any of the situations you may have run into in the past.

The point is, there are mechanisms to achieve the functionality you're trying to achieve, and the approach taken here chooses not to take them.

If you'd like to re-work the patch to use the proposed frameworks, we'd love to help point you in the right direction and assist with the effort. You can continue the discussion of those approaches on the asterisk-dev mailing list or in #asterisk-dev.





- Matt Jordan


On April 23, 2014, 9:02 a.m., Vitezslav Novy wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3471/
> -----------------------------------------------------------
> 
> (Updated April 23, 2014, 9:02 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23636
>     https://issues.asterisk.org/jira/browse/ASTERISK-23636
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch introduces another approach to dynamically controlled MoH.
> Unlike realtime this way is file based.
> 
> As a switch between normal and alternative behavior, boolean variable
> 'dynamic' is used in MoH config file.
> 
> By setting
> 
> dynamic=yes
> 
> new behavior is switched on.
> 
> How dynamic behavior works
> 
> All static MoH classes in musiconhold.conf and realtime are ignored, except [default]
> class. On the other hand dynamic class named 'default' is ignored too.
> 
> New variable 'dynamic_dir' defines directory, where dynamic classes are
> defined. Each first level subdirectory of dynamic_dir defines one MoH class
> with same name as directory name.
> If class directory contains playlist file 'playlist.txt' content of
> the file defines audiofiles in class and their order. Otherwise directory
> is scanned same way as for standard MoH class with mode=files.
> 
> Playlist expects one file on line, without path and without extension.
> Files must be placed in class directory.
> If first line of playlist contains exactly one character '%', files will be
> ordered randomly.
> 
> 
> Diffs
> -----
> 
>   /trunk/res/res_musiconhold.c 412900 
>   /trunk/configs/musiconhold.conf.sample 412900 
>   /trunk/CHANGES 412900 
> 
> Diff: https://reviewboard.asterisk.org/r/3471/diff/
> 
> 
> Testing
> -------
> 
> Original 'static' behavior with dynamic=no
> Dynamic class without playlist
> Dynamic class with playlist
> Random ordering with % on first line of playlist 
> % as audio file name
> 
> 
> Thanks,
> 
> Vitezslav Novy
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140425/e0c7999e/attachment-0001.html>