Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

per-message TTL header name #146

Open
poohsen opened this issue Jul 5, 2018 · 3 comments
Open

per-message TTL header name #146

poohsen opened this issue Jul 5, 2018 · 3 comments

Comments

@poohsen
Copy link

poohsen commented Jul 5, 2018

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
Copy link
Member

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
Copy link
Author

poohsen commented Jul 6, 2018

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants