Updates RabbitMQ logic

[chriswiggins]

The RabbitMQ logic inside Janus followed some conventions that were non-standard. I suspect the author wasn't completely across the way AMQP is supposed to work, which is very easy to do if you don't work with it every day! (I say that from experience 💯 )

• Publishing to a topic does not require an outgoing queue, just the topic, so the outgoing queues are no longer declared
• When the janus_exchange_type is topic, we want to be able to name the queue, and then bind an incoming topic from the exchange to that queue, so that functionality has been added (see queue_name variable)
• Adds options to allow configuring the following queue options: durable, autodelete, exclusive
• This is all backwards compatible, and won't break existing applications

I understand this is quite a big change, so happy to discuss it in depth. To note also I've moved some of the logic/config around so that it follows exchange/queue/topic process

I'm also still fully testing this, but wanted to open the PR to get initial feedback

[januscla]

Thanks for your contribution, @chriswiggins! Please make sure you sign our CLA, as it's a required step before we can merge this.

[atoppi]

@chriswiggins is this still a draft ?

[chriswiggins]

@atoppi yes - I'd like to add the ability to make queues exclusive and autoDelete, so will continue working on that until this gets merged in.

[chriswiggins]

This is ready now - I haven't been able to test this with the default fanout exchange, is there a test environment that can be used for that?

[lminiero]

I suspect the author wasn't completely across the way AMQP is supposed to work

That would be me, and that's a realistic assumption 😉

That said, it looks like your PR will be a breaking change: anyone using the RabbitMQ transport plugin in production right now (and I know some companies that do), will find themselves with something that doesn't work anymore, which is why I don't think we can merge it as it is, even if I'm sure it's a considerable enhancement on best common practices.

The default behaviour should still be there, and probably still be the default, at least for a little while (possibly with a big warning on the logs that says it will change sooner or later), while more savvy users can enable your improved support.

[atoppi]

In general this is a nice enhancement from the previous implementation.
However I've added some comments in order to make it back-compatible with existing deployments.

[chriswiggins]

Thanks for the feedback @atoppi ! Agreed I haven’t left this completely backwards compatible and the last thing we want is to break production for anyone.

I’ll get on to making those changes early next week

[chriswiggins]

@atoppi do you want to have another check now? I've added an option to override the default behaviour of declaring the outgoing queues

[lminiero]

Thanks for the changes! We're busy at the IETF meeting, so we may not be able to check until next week.

[atoppi]

I have added some more comments, see if they all make sense to you.

[lminiero]

@chriswiggins as soon as Alessandro's notes are addressed, I'll make one more review and possibly a small test as well, as I believe we're getting close to this being merged. Thanks again for the improvements!

[lminiero]

@chriswiggins any updates on the above? Looking forward to review and merge soon 😁
Thanks!

[lminiero]

@chriswiggins please notice that I'll have to close the PR if we don't get any feedback soon, as we'll have to assume you've moved on and are not interested in this anymore otherwise (we haven't seen activity from you for 3 weeks). As such, if you'd like to see this merged please address the changes and we'll take care of that. Thanks!

[chriswiggins]

Hi team, yes definitely keen on fixing this up as we're actually already using these changes in our production environment. Unfortunately I've got a huge deadline to hit this week so not likely I'll be able to get to this until next week. Is it ok if it waits until then?

[chriswiggins]

Just an update - I still want to make all the changes and fixes mentioned above, and will do so, but we've been scrambling to move all our video across to Janus before Google Chrome removes flash in the end of December 😨

This will likely be something I work on when on holiday in January. There's also no RabbitMQ reconnect logic for API comms, where there is for the event channel, so I might fix that too, but will do that in a separate PR.

Happy Holidays to all, and thanks again for such an awesome project with Janus. It really works incredibly well

[chriswiggins]

Happy 2021 to all - hopefully the latest commit addresses the issues discussed.

Let me know what else is required?

[lminiero]

Thanks @chriswiggins! We'll review and test later today, and merge in case 👍

[atoppi]

@chriswiggins have you checked that everything works as expected ? (queues declared etc.)

[chriswiggins]

@chriswiggins have you checked that everything works as expected ? (queues declared etc.)

I have checked a couple of use cases, is there some form of test suite we can follow to ensure the original functionality doesn't break?

[atoppi]

There is no test suite for this case.
It would be enough to check if the default configuration keeps the same topology as before.

[chriswiggins]

There is no test suite for this case.
It would be enough to check if the default configuration keeps the same topology as before.

Hey @atoppi - I've verified that by default all stays the same 👍

[atoppi]

lgtm, merging.
@chriswiggins thanks for the effort!

[lminiero]

Thanks Chris!

[lminiero]

I guess I should have checked this myself too, instead of trusting the automated builds...

transports/janus_rabbitmq.c:912:5: warning: implicit declaration of function 'strlcpy'; did you mean 'strncpy'? [-Wimplicit-function-declaration]
  912 |     strlcpy(incoming_topic, (char *)d->routing_key.bytes, d->routing_key.len + 1); // Convert the amqp_bytes_t back to char*

which leads to

[ERR] [janus.c:main:5335] 	Couldn't load transport plugin 'libjanus_rabbitmq.so': /home/lminiero/Work/code/test/github/lib/janus/transports/libjanus_rabbitmq.so: undefined symbol: strlcpy

I'll work on a fix shortly.