Master-Slave Aggregating Relay (multiple master setup) #5
Conversation
|
Please do not include the |
|
So, sorry for getting right to the point on my first comment, it was just one thing that I noticed immediately. (If it's too much hassle to remove it, don't worry, I can do that manually later) First of all, thanks a lot for the pull request! It needs some review and discussion and, of course, some testing before we can decide if we can merge it. |
src/slave.c
Outdated
| * the master linked list and releasing the lock */ | ||
| master = config->master; | ||
| while (master) { | ||
| update_from_master(master); |
There was a problem hiding this comment.
This seems indeed problematic to do, as it can hold the config lock for quite some time, if fetching the streamlist takes a while or you have a lot of master servers.
There was a problem hiding this comment.
OK ... Any recommended approach?
I'll start by copying the master linked list out of the config and releasing the lock, then iterate the copied linked list.
Approaching it in a similar manner to the relay linked list might be preferable though, in that the copied master linked list (eg. global.master) would only have to be updated when the config changes.
We can basically aggregate all streams from any number of master servers
Added an example of a master relay server aggregation
At this point is it more for ease than anything else. The locks could be released quickly with some effort.
Config locking during update_from_master is now avoided. This method is still inefficient as the copy is performed each master-update-interval when it could be done only when the config changes
|
Removed that I have also implemented the copy master servers list from config change to minimize config locking. |
No need to check for NULL list param when calling
Thanks, I think this is better now. Will still need to discuss this with the other project members which can take a while, just wanted to inform you, so that you know that I haven't forgotten your PR. 😄 |
|
@greenbender - with this patch & the implementation of namespaces, would master and slave servers have different visibly (to listener clients) named mountpoints (such as with a configured namespace of "upstream1-", /mount.opus on master would become /upstream1-mount.opus on slave server(s) visible to listeners? I ask because in my use case, having the same visible mountpoint name on all servers, master or slave, would be important to maximize resources (sources could use any one of the servers in the "mesh" and have their configured mountpoints relayed by all other servers with the same mountpoint name for all listener clients - this would be important for, say, sharing their stream URL). master-slave-aggregation is an incredibly useful feature addition IMHO and look forward to using it =} Thank you. |
|
At the moment the namespace is simply pre-pended to the mountpoint so in your example your might make the namespace "/upstream1" (yes a leading forward slash - thinking of making that optional?) and so the mountpoint "/mount.opus" on the master would become "/upstream1/mount.opus" on the slave. The "mesh" sounds more like a use case where all streams would have to be uniquely named and you wouldn't use a namespace (If I understand correctly) then all servers would have a complete set of mountpoints for the rest of the "mesh" - Its been a while since I wrote this (I'm not sure what happens when a mountpoint path is already in use, does it get ignored/error/override). Perhaps I'm misunderstanding your requirements? |
|
Oh, I see how it works ("/upstream1/mount.opus", not with a hyphen). That makes a lot of sense. I do know that, without master-slave aggregation when you try to make a master and slave relay each other with duplicate mountpoint names, weird things happen(tm). I think you answered my question with your first comment regarding the pathname of the aggregated mountpoint, but let me ask this: would it be hard to, on the master with this feature addition, to create a namespace for its own (local, non-relayed) mountpoints? Or probably this would yield the same results as you mentioned above.. My scenario is that I want to point both source and listen Icecast DNS hostnames dynamically (via a CNAME). With this setup I'd like the listen URL from any source on any server, to be the same for all listeners. Also, seems like listener clients wouldn't have a problem digesting a nested mountpoint like this? Thanks for your response! =} |
That shouldn't be a problem for clients, as it's just a HTTP resource to them. |
There was a problem hiding this comment.
Thanks for this patch! Only have two nitpicks so far but thats it from my side. I guess we would still need to implement the master list properly before we can ship this though, as copying all the time is not really good, but you already stated this yourself in a comment in the code.
src/cfgfile.c
Outdated
|
|
||
| master->next = NULL; | ||
| master->on_demand = configuration->on_demand; | ||
| master->server = (char *) xmlCharStrdup("127.0.0.1"); |
There was a problem hiding this comment.
Why are you doing this? It does not seem right.
There was a problem hiding this comment.
It seemed like a reasonable default at the time master->server = NULL would also be fine though it should perhaps be logged as a bad config if master->server is not set.
src/slave.c
Outdated
|
|
||
|
|
||
| /* free master list and return NULL */ | ||
| static master_server *master_list_free(master_server *list) |
There was a problem hiding this comment.
I would not return NULL here but make it void. It is a smart idea but personally I think it makes code that comes later where you do:
foo = master_list_free(foo);
a bit confusing if one is not familiar with this behavior of master_list_free.
There was a problem hiding this comment.
I have implemented this change
Rather than default to 127.0.0.1 make <server> a required field in <master> and warn if it is not supplied. Any master that has a NULL server will be skipped during updates. Also ensure that server is non-NULL before freeing.
This makes things a bit more explicit where the function is called
When a namespace is supplied for a master ensure that the URI for the localmount starts with a forward slash. When the namespace already starts with a forward slash use it as is, otherwise prepend a forward slash to the localmount.
Even though memory is calloc'ed for the master structure being explicit is probably a good idea.
|
Why there is no activity on this enhancement ? Would be great to have this feature. |
|
Does this need to be transferred to gitlab ? |
|
@greenbender thanks for this - I am looking for this feature as well but it's not yet been merged. I note your source is built on 2.4.1. Do you happen to have a version of this based on 2.4.4 that I could use and build from source? Many Thanks! |
This pull request adds support for multiple Master-Slave relays. Something I am calling a Master-Slave Aggregating Relay.
A couple of people have asked for this feature in the past and were referred to the Single-Broadcast Relay, which is OK provided you know the specific mountpoints that you want to relay ahead of time.
If you are in a position where you want to relay mountpoints for more than one the master server, and one or more of the master servers adds and removes mountpoints dynamically or if you are unable to know what mountpoints are available ahead of time then this gives you the ability to aggregate all mountpoints for any numbers of master servers.
Namespacing is also supported to prevent name clashes for mountpoints being relayed from different servers.