SpinGo / op-rabbit

The Opinionated RabbitMQ Library for Scala and Akka
Other
232 stars 73 forks source link

per-message TTL header name #146

Open poohsen opened 6 years ago

poohsen commented 6 years ago

I think there is an issue with ModeledMessageHeaders.x-message-ttl. I tried using it on a message and couldn't verify that the message actually got dropped from the queue. Looking at the docs (https://www.rabbitmq.com/ttl.html#per-message-ttl-in-publishers) I would say that the actual header name should be expiration and not x-expires. The latter seems to be only for setting queue TTL values. I think this is not very consistent of RMQ but that's how it is.

Using expiration (via AMQP.BasicProperties.Builder().expiration("60000").build()) I actually get the dropping of dead messages that I expect.

I'm happy to provide a PR if you confirm my interpretation of the behaviour.

timcharper commented 6 years ago

x-message-ttl is valid for queues, but not for messages, then? Op-rabbit doesn't (currently) attempt to validate if a header is applied in the appropriate context, but such could be done by mixing in traits (which would be a breaking change, so, ideally we just do it in a way that generates compiler warnings first).

I'd be willing to accept such a PR

poohsen commented 6 years ago

UPDATE: while testing the PR I saw that expiration isn't actually a header, it's another property field on the message. And you already modelled it as com.spingo.op_rabbit.properties.Expiration but the class description claims that RMQ doesn't interpret this property, which is false. So by simply using using Message(data, publisher, Seq(Expiration("60000"))) i can get a message TTL of 1 minute.

So my current proposal is to drop ModeledMessageHeaders altogether and adjust the comment on that property. I'll put that in the PR.

============================ AFAICT from the docs, there are 3 distinct concepts and all have their own header names:


Op-rabbit doesn't (currently) attempt to validate if a header is applied in the appropriate context

That's true. But there is already an object ModeledMessageHeaders and when using it one expects to achieve TTL behaviour on a per-message basis:

object ModeledMessageHeaders {
  import properties._

  /**
    This message header causes RabbitMQ to drop a message once it's reached the head of a queue, if it's older than the provided duration.

    [[http://www.rabbitmq.com/ttl.html#per-message-ttl Read more]]
    */
  val `x-message-ttl`: UnboundTypedHeader[FiniteDuration] = UnboundTypedHeaderLongToFiniteDuration("x-expires")
}

link

Based on the RMQ docs linked above, I'd say this per-message header uses the wrong header name. It should be expiration and not x-expires since the latter is for queue-TTL. So this would be the first thing to fix.

I'd also rename that val to expiration.

As for enforcing the usage of the right type of header in the right context I'd have to give it a go and see how far I get, but I see it as an optional library improvement while fixing that line above is the actual bugfix.

timcharper commented 6 years ago

Thanks for the detailed investigative report. I think you're right, it should be ModeledMessageProperties. Also, it seems like using the single name "Header" may not have been ideal, it would probably be better to have TypedHeader, and TypedProperty, etc. It might be substantial work to do so, but once done an implicit conversion with a deprecated warning can be put in place so that code continues to compile to assist with upgrade.