search

rabbitmq / rabbitmq-autocluster

RabbitMQ peer discovery and cluster formation plugin, supports RabbitMQ 3.6.x
BSD 3-Clause "New" or "Revised" License
242 stars 54 forks source link

Add startup lock support for Consul backend #20

Closed ValFadeev closed 7 years ago

ValFadeev commented 7 years ago

I actually began working on this one on the back of https://github.com/rabbitmq/rabbitmq-autocluster/pull/6, and it is sort of coming together, hence opening an issue to check if there is any work lined up or done already to avoid duplicated effort. Otherwise happy to submit for an early review in a few days.

michaelklishin commented 7 years ago

@ValFadeev we are fairly close to accepting #6. The biggest question for us is how much of that stuff can go into RabbitMQ core for 3.7.0 (some things in #6 are pretty opinionated and specific to a particular company).

That's why having an implementation for Consul which is as minimalistic as possible would be super helpful for us. Currently it's someone hard to see the forest behind the trees in #6 with all the extra node filtering stuff that's added.

michaelklishin commented 7 years ago

FTR, a Consul-based backend for RabbitMQ master is something I've been working on recently: https://github.com/rabbitmq/rabbitmq-peer-discovery-consul. So far the only two things that are obviously missing is the randomized startup delay stuff from rabbitmq-autocluster (will be in RabbitMQ proper) and tests.

Locking can still be included. Key question I see is how should locking be reconciled with randomized startup delays. Perhaps there is an answer for that in #6, I personally haven't really reviewed it yet.

michaelklishin commented 7 years ago

Oh, and I'm not aware of any parallel efforts for rabbitmq-autocluster itself since we haven't even merged #6 yet :)

ValFadeev commented 7 years ago

great, thanks! then I'll get my patch to a reviewable state and hopefully take it from there

michaelklishin commented 7 years ago

Sounds great, thank you!

ValFadeev commented 7 years ago

Tried this in my staging environment, seems to be doing the job

ValFadeev commented 7 years ago

@michaelklishin wondering how things are looking for https://github.com/rabbitmq/rabbitmq-autocluster/pull/6? Keen to finish the tests and any fixes that may be necessary in https://github.com/rabbitmq/rabbitmq-autocluster/pull/22

michaelklishin commented 7 years ago

@ValFadeev it's under a review. Chances are it will be merged with minimal changes. Unfortunately we can't move it as is to RabbitMQ core but the locking part is definitely under consideration.

Thank you!

michaelklishin commented 7 years ago

@ValFadeev I know it may be a lot to ask but while we review #6 and #22, the most productive thing a contributor can do is to look into the peer discovery mechanism we have in master and experimenting with how [un]locking functions can fit there. This plugin isn't going away even after 3.7.0 but the long term goal is to make it largely irrelevant.

ValFadeev commented 7 years ago

sure, I will try to set aside some time for that

michaelklishin commented 7 years ago

@ValFadeev cheers. Just in case: by "master" above I mean RabbitMQ server master, not this plugin's.

michaelklishin commented 7 years ago

22 is in.