Separating topology definition from use

[mxriverlynn]

Per conversation in #2, I've made a first attempt at pulling the topology definition out of the message producer and consumer base types.

I'm looking for feedback on the current implementation and requirements, for the code in this pull request.

Drop-In Replacement

At the moment, this should be a drop-in replacement for current Rabbus use and I want to keep it that way as much as possible... but I recognize that this may not be possible, given some of the code cleanliness issues I've noted below.

Right now, you'll still be able to write code like this:

new Rabbus.Sender(wascally, {
  exchange: exchangeConfig,
  messageType: msgType,
  routingKey: rKey
});

This code will create the topology for you, internally, as Rabbus does now.

Execute Extracted Topology

What this change provides, though, is the ability to separate the topology configuration so that your message producers and consumers are not required to define the topology directly.

For example,

// define the topology
// -------------------------

var topology = new Rabbus.Topology(wascally, {
  exchange: exchangeConfig,
  messageType: msgType,
  routingKey: rKey
});

// execute the topology against rabbitmq
// ---------------------------------------------------

topology.execute(function(err){

  // now use the topology in a message producer or consumer
  var sender = new Rabbus.Sender(wascally, topology);

  // ...
});

Use Pre-defined Topology

With the ability to extract the topology definition, you also have the option of not executing it against RabbitMQ. This would allow you to use existing topology definitions in your RabbitMQ server, and use the wascally configuration options to a greater extent.

// define the topology
// -------------------------

var topology = new Rabbus.Topology(wascally, {
  exchange: exchangeConfig,
  messageType: msgType,
  routingKey: rKey
});

// don't execute it because we know rmq already has it
// just use the topology, as is
// ---------------------------------------------------------------------

var sender = new Rabbus.Sender(wascally, topology);

Potential Areas I Don't Like

There are a few things I don't like about the way this works, right now, most of which is out of my control I think.

Passing wascally Everywhere

First off, you have to pass a wascally object into the Topology and the Sender, as shown above.

This is needed in the Topology so it can wascally to define the topology on RMQ. It's also needed in the Sender so that it can send the message across.

It might be nice to not require passing wascally to every object instance, all the time ... but this seems a minor thing.

Required Message Type For Topology

Due to the way wascally works with message types, at the moment, you're required to provide a message types, you're still required to provide a message type for a producer and consumer topology configuration, even if this is a predefined topology.

var preConfigSendTop = new Topology(rabbit, {
  exchange: ex1,
  messageType: msgType1,
  routingKey: rKey
});

var sender = new Sender(rabbit, preConfigSendTop);

var preConfigRecTop = new Topology(rabbit, {
  queue: q1,
  messageType: msgType1
});

rec = new Receiver(rabbit, preConfigRecTop);

This requirement for a message type may change with the next major release of wascally, but i haven't tested it against that new fork/branch, yet.

Required Routing Key For Message Producer

With the current code, you must provide a Routing Key to the topology for a message producer.

var preConfigSendTop = new Topology(rabbit, {
  exchange: ex1,
  messageType: msgType1,
  routingKey: rKey
});

var sender = new Sender(rabbit, preConfigSendTop);

This doesn't make sense to me, from an API perspective. It's the Sender that needs to know about the routing key (and message type, for that matter), not the topology.

I had a hard time keeping the code implementation clean when I tried to move the routingKey and messageType into the message producer base type

Misc Other Bits

There are probably some misc other bits in the code that I'm not super comfortable with, mostly related to separation of concerns like the message type and routing keys, and/or keeping code clean.

Feedback Needed

I'd like to get feedback on the code in this pull request, the items I've noted above, etc.

You can see the new Topology object here: https://github.com/derickbailey/rabbus/blob/topology/rabbus/lib/topology/index.js

And there are basic specs for re-using existing topology here: https://github.com/derickbailey/rabbus/blob/topology/rabbus/specs/topology.specs.js

Use of the topology in the base producer and consumer are here:

• consumer: https://github.com/derickbailey/rabbus/blob/topology/rabbus/lib/consumer/index.js#L84
• producer: https://github.com/derickbailey/rabbus/blob/topology/rabbus/lib/producer/index.js#L89

[shurik239]

Sorry, i did not understand the original problem, that you want to solve with this PR. Is that the race conditions by starting produce message just after defined consumer for it? Small quote from #2

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.

To avoid this issue for my project, I am trying to wrap your consume method, and return promise, that will be resolved, only when consumer emit "ready" event.

[mxriverlynn]

@shurik239 -

i did not understand the original problem, that you want to solve with this PR.

i should have explained in the PR description, or provided a better description in the original ticket. sorry about that - here's a brief overview of the problem:

it's possible (and common) for wascally or rabbitmq itself to have pre-configured topology that needs to be re-used, without duplicating the complete topology definition in rabbus.

For example, wascally allows you to specify a complete topology layout with it's configure method:

wascally.configure({
  connection: { ... },

  queues: [
    { name: "some.q" }
  ],

  exchanges: [
    { name: "some.ex" }
  ],

  bindings: [
    { exchange: "some.ex", target: "some.q", routingKey: "a.key" }
  ]
}).then(...);

In the current version of Rabbus, with wascally is configured in this manner, you would have to re-declare the entire topology in the Rabbus configuration objects:

var sender = new Rabbus.Sender({
  exchange: "some.ex",
  routingKey: "a.key"
});

var receiver = new Rabbus.Receiver({
  exchange: "some.ex",
  queue: "some.q",
  routingKey: "a.key"
});

This original API design was needed for my own work where I have a lot of queues that are generated at runtime, based on configuration in the application. I honestly don't use pre-configured topology very often. But, I do have a few places where it would make sense.

More importantly, other Rabbus users are wanting the simplicity of both pre-configured topology w/ wascally and still use Rabbus. But no one wants to duplicate the topology definition, and no one wants the topology definition to execute against RabbitMQ twice.

This pull request attempts to address that problem by extracting a Topology object from the current Rabbus producer and consumer base types. The goal of the Topology object is to allow use of existing topology definitions in RabbitMQ / wascally, without re-declaring the whole thing or re-executing the topology definition against RabbitMQ.

Hopefully that makes more sense. :)

...

Regarding the lost messages issue that you're looking at, and the wrapper you've created around the "ready" event, that should probably be a separate ticket. Even though it was originally part of #2, I think there's enough divergence in that issue vs this issue to warrant a separate ticket. It definitely needs attention - at least to make sure the scenario is documented, if not handled properly.

[shurik239]

Thank you, @derickbailey ! As usual very clear reasoning from you. :)

As far as the "old" style of defining Producers and Consumer is working (instancing Topology in background), this changes is fine for me. Are you planning to make that deprecated?

[mxriverlynn]

Are you planning to make that deprecated?

Yes, but not right away.

The first release w/ the new Topology may not have a deprecated message, but at some point I want to add a message saying the old configuration style is deprecated and that you should pass in a Topology object directly.

Having that as a required parameter will clean up a bunch of code and generally make things more flexible for the future. But I want to see how some of the other api design issues play out, surrounding the routing key and message type, first.

[rhyslbw]

Before getting too far into the weeds, have you considered Rabbus as a more generic service bus that just happens to support the use of wascally as a transport infrastructure? Seems like this would solve some of the pain points relating to configuration and give us more freedom with the API to not have to conform to wascally. Of course this also has the benefit of potential support for other tech, but that's more of a side-effect IMO.

[mxriverlynn]

@rhyslbw yeah, i've thought about that more and more as i'm moving the abstraction of rabbus forward. at this point, i don't want to commit to that because it introduces another very large amount of code to either do everything wascally does for rabbus, or create an adapter layer that wascally can fit into. i don't like either of those situations right now. maybe at some point in the future, it will be obvious which one of those paths will make sense, but for now i want to see where we can take this with wascally. ... especially knowing what's coming down the line soon, with the next major release of wascally fixing some of the issues i have with it

[rhyslbw]

@derickbailey Fair point, in that case some feedback:

I honestly don't use pre-configured topology very often. But, I do have a few places where it would make sense.

Having thought about this further I actually think the real feature here is to address this:

… and no one wants the topology definition to execute against RabbitMQ twice.

… as wascally config APIs are called from within the producer and receiver objects.

If using Rabbus and wanting to establish some other topology that doesn't relate to producing or consuming messages, (like a dead-letter exchange to at least catch rejected messages until the appropriate resolution handler can be implemented), it's maybe of no concern to the application anyway right? This seems most appropriate to be done directly with the Rabbit server (bash script or web GUI) , and then Rabbus could take over the responsibility of managing this topology via a receiver if we wanted to later on programatically process those dead letters.

I do think moving out topology initialization from the producers and consumers will make it easier to reason about what the app is doing though in addition to what I mentioned above.

With the current code, you must provide a Routing Key to the topology for a message producer…This doesn't make sense to me, from an API perspective. It's the Sender that needs to know about the routing key (and message type, for that matter), not the topology.

Considering the topology is the like the message flow schema, and that part of initializing this is binding a queue to an exchange with routing keys… it makes sense to me :smile: Am I missing something?

I think we can leave any issues with messageType alone and deal with that later.

[mxriverlynn]

I think we can leave any issues with messageType alone and deal with that later.

+1

it seems the people that have weighed in on this pull request are generally in agreement of the direction it heads. i want to look at cleaning up the API and internals a bit, but i think this will be the way forward.

i also want to look at the v.next release of wascally that is happening outside of the main wascally tree. there are some good things that will hopefully help what we're doing w/ rabbus