mxriverlynn / rabbus

A micro-service bus with built-in messaging patterns, for NodeJS and RabbitMQ
116 stars 26 forks source link

How are we going to use rabbus if we have a pre-defined topology via JSON configuration method? #2

Closed rtorino closed 8 years ago

mxriverlynn commented 9 years ago

at the moment, rabbus would attempt to recreate the same topology that you have defined in your JSON file. It would be a duplication of effort, really - defined once in the json config and again in the rabbus objects.

i don't use wascally like that, personally. i let my rabbus objects define the topology dynamically, due to my system requirements.

i can see the json config being useful, though. are you interested in putting together a pull request to handle this? i can offer pointers and help, as needed, of course. if you won't be able to, i'll look in to it when i have a chance.

rtorino commented 9 years ago

I see. We just wanted to prevent potential race conditions when creating queues and exchanges that could possibly happen if we are going to create the queues and bindings in demand/dynamically. Yup! I'm interested in putting a pull request to handle this and any pointers and help will be appreciated.

mxriverlynn commented 9 years ago

Yeah, I can see how that would be good. Even with the explicit subscription starting, it would be possible to have a message publisher pushing messages before a subscriber has configured the queue and exchange bindings. That would lead to lost messages.

My first thought on how to approach this, is that each of the object has a _start method in them, which configures everything. https://github.com/derickbailey/rabbus/blob/master/lib/sender.js#L22 for example. You could modify the _start methods to check for a "preConfigured" option or something along those lines. If it finds that, skip the topology configuration and just resolve the promise right away.

There are a lot of ways to make this work, of course. That was just an off-the-top-of-my-head idea. :)

mxriverlynn commented 9 years ago

also - you'll want to run the tests for this, as your working. npm install should get everything in place for you, and grunt will run the tests and watch for file changes to re-run them. if you look in the specs/config/index.json file, you'll see the connection information that i use for rabbitmq, in the tests.

rhyslbw commented 8 years ago

any updates on this? I'm wondering the best way to configure dead letter queues

mxriverlynn commented 8 years ago

no updates... this has been on my mind, but i haven't looked into a good solution for it, yet.

regarding dead letter queues, rabbus supports them already... rabbus will pass-through any options you provide on declaring a queue, including wascally's deadLetter option:

new Subscriber({
  queue: {
    // ...
    deadLetter: "some.dlx"
  }
});
rhyslbw commented 8 years ago

Thanks for the hint @derickbailey. If I can think of a design I'll be able to contribute to this issue, as the potential for lost messages without explicit topology is a little concerning :frowning:

mxriverlynn commented 8 years ago

yeah, i understand... there are times when i don't want to dynamically declare a queue / exchange / etc either... but i live with it for now.

my first thought is possibly a "preconfigured" flag (or something with a better name) that would be passed into the object definition:

new Subscriber({
  queue: "my-queue-name",
  preconfigured: true
});

and this flag would cause the code to bypass the part where it tries to configure the queue, exchange and binding.

i don't like the name "preconfigured", necessarily - i'm open to ideas. but i think something like this would not be too hard to do.

rhyslbw commented 8 years ago

i don't like the name "preconfigured", necessarily - i'm open to ideas. but i think something like this would not be too hard to do.

how about just configure that defaults to true?

mxriverlynn commented 8 years ago

hmmm... possibly... still doesn't quite sound right to me.

ideas off the top of my head, of words to combine to find the right setting name:

something to give this setting some meaning in a verb and noun combination

...

oh, i just had a thought: what if topology configuration were separated from service bus object configuration?

instead of having Subscriber contain all of the code that is required for building the topology, have a Topology object or something similar. this would handle the construction of the topology so the other objects can use it.

then we could either change things so that you have to build a topology before creating the object, or maybe have the Subscriber use a default topology object which can be replaced

so, the existing code like this would still work:

new Subscriber({
  queue: "some.queue",
  exchange: "some.exchange",
  routingKey: "some.key"
});

this code would recognize the object literal, create a Topology object for you, and execute it against rabbitmq

alternatively, you could pass in your own topology object:

var top = new Topology({
  queue: "some.queue",
  exchange: "some.exchange",
  routingKey: "some.key"
});

// call 'create' to do the actual topology creation in rabbitmq
top.create((err) => {
  new Subscriber(top);
});

with this, you could create a Topology object, and just pass it directly into the Subscriber without calling create and the Subscriber would just use the topology configuration that you supplied.

var top = new Topology({
  queue: "some.queue",
  exchange: "some.exchange",
  routingKey: "some.key"
});

// don't `create` the topology. just use it, assuming
// the topology already exists in rabbitmq

new Subscriber(top);

I like this idea (or something similar). it would clean up some really nasty parts of the code, provide far more flexibility within the use of rabbus, fix this particular problem, and more.

what do you think?

rhyslbw commented 8 years ago

what if topology configuration were separated from service bus object configuration?

This would be a great idea, and really is the natural way to proceed. My only though relating to your design is to omit the default top object and require the developer to always manage the topology creation explicitly. It will simplify the documentation, encourage better architecture, and remove some of the magic that can trip you up.

mxriverlynn commented 8 years ago

omit the default top object and require the developer to always manage the topology creation explicitly. It will simplify the documentation, encourage better architecture, and remove some of the magic that can trip you up.

i generally agree with this... but i'm also thinking about easy upgrade paths.

the first version of this will probably do a topology for you, so you can drop-in replace the new version of rabbus. but i could have it produce a console warning message to let you know that this is a deprecated feature, and won't be allowed in the future.

mxriverlynn commented 8 years ago

I've started work on this, which can be seen in the topology branch. you can see the specs i'm working from in https://github.com/derickbailey/rabbus/blob/topology/rabbus/specs/topology.specs.js

it's not as nice as i wanted, due to some limitations of how wascally works and how rabbus is set up... but i think it's a good step in the right direction. what do you think?

rhyslbw commented 8 years ago

Sorry for the delay, I had to travel interstate for a job.

Looks good on the whole, if you open a PR I can make inline comments in context.

mxriverlynn commented 8 years ago

I've created a pull request at #21 to show the new code, the things I'm not currently happy with, etc.

mxriverlynn commented 8 years ago

v0.8.0 now includes the topology object and definition