twingly / twingly-amqp

:bus: Ruby gem for RabbitMQ subscribing and publishing
0 stars 0 forks source link

Add functionality for publishing delayed messages #93

Closed roback closed 3 years ago

roback commented 3 years ago

Started with one idea (a separate class), then continued with another idea in b267a08835a32aaa38851a1f3079c845fc3d3ba0 (doing it in the DefaultExchangePublisher class), so no need to check this commit by commit. I'll just squash this before merge.

See:

walro commented 3 years ago

README?

roback commented 3 years ago

README?

I was waiting to see if this looked reasonable before adding any documentation. Mostly the name DelayedDefaultExchangePublisher, as I couldn't come up with anything better :)

walro commented 3 years ago

I was waiting to see if this looked reasonable before adding any documentation. Mostly the name DelayedDefaultExchangePublisher, as I couldn't come up with anything better :)

I was thinking it's a bit easier to understand how it's meant to be used if one sees an example of it

walro commented 3 years ago

What if we just exposed a method of creating the "delay queue"? Then you publish to it, using one of the existing publishers (guessing one would use DefaultExchangePublisher)?

roback commented 3 years ago

What if we just exposed a method of creating the "delay queue"? Then you publish to it, using one of the existing publishers (guessing one would use DefaultExchangePublisher)?

That would work, as long as you remember to set options.expiration (and as you said, you need to use the DefaultExchangePublisher). My thinking was that the tests in the projects that uses this gem will be a bit simpler if we have a specific class for it. Otherwise we would need to both test that we create a delay queue and a publisher with the correct options. Perhaps the difference is minimal...

walro commented 3 years ago

I was just thinking it seemed like a lot of "extra things" implemented here when we already had the publishing part (I didn't think of expiration thing though).

Would it be possible to "composite" the parts we already have somehow, like "creating the magic queue" part, we already have the publisher part and then add the expiration part.

roback commented 3 years ago

Would it be possible to "composite" the parts we already have somehow, like "creating the magic queue" part, we already have the publisher part and then add the expiration part.

Probably, I'll try that

roback commented 3 years ago

I can't really come up with any better way to do this. Most of the publishing logic comes from the Publisher module, so not much is duplicated, but sure, there's now a new class which essentially does the same thing as the DefaultExchangePublisher (except for the queue creation part that is).

walro commented 3 years ago

Alright, I'll try to take a jab at it too (hopefully!)

roback commented 3 years ago

One idea. I added a DefaultExchangePublisher.delayed method that creates a queue and returns a new DefaultExchangePublisher with expiration set. Of course the queue creation should be moved to a separate class, but this way we at least don't need a separate class for the publisher.

diff --git a/lib/twingly/amqp/default_exchange_publisher.rb b/lib/twingly/amqp/default_exchange_publisher.rb
index 273eace..bc0d2d5 100644
--- a/lib/twingly/amqp/default_exchange_publisher.rb
+++ b/lib/twingly/amqp/default_exchange_publisher.rb
@@ -9,6 +9,31 @@ module Twingly
         connection ||= Connection.instance
         @exchange = connection.create_channel.default_exchange
       end
+
+      def self.delayed(delay_queue_name:, delay_ms:, target_exchange_name: "",
+                       target_routing_key: "", connection: nil)
+         if [target_exchange_name, target_routing_key].all?(&:empty?)
+           raise ArgumentError, "At least one of target_exchange_name or " \
+                                "target_routing_key must be set"
+         end
+
+         connection ||= Connection.instance
+         connection.create_channel.queue(delay_queue_name,
+           durable: true,
+           arguments: {
+             "x-dead-letter-exchange":    target_exchange_name,
+             "x-dead-letter-routing-key": target_routing_key,
+           },
+         )
+
+        new(queue_name: delay_queue_name, connection: connection).tap do |publisher|
+          publisher.configure_publish_options do |options|
+            options.expiration = delay_ms
+          end
+        end
+      end
     end
   end
 end
walro commented 3 years ago

One idea. I added a DefaultExchangePublisher.delayed method that creates a queue and returns a new DefaultExchangePublisher with expiration set. Of course the queue creation should be moved to a separate class, but this way we at least don't need a separate class for the publisher.

That sounds pretty good!

walro commented 3 years ago

Conflicting files

lib/twingly/amqp/default_exchange_publisher.rb

🙀