Fix AddArgument, Add Queue TTL config

[ronnyek]

Summary of changes:

• Fixed the AddArgument property of the RabbitMqQueueOptionsBuilder to allow passing object values in (simply setting all the arguments would have allowed this).
• Added additional configuration parameter that sets x-expires argument before calling queueDeclare.

I ran all unit tests before and after and saw no differences in success. Made sure to document the method and incorporate argument checking as well

---

Rebus is MIT-licensed. The code submitted in this pull request needs to carry the MIT license too. By leaving this text in, I hereby acknowledge that the code submitted in the pull request has the MIT license and can be merged with the Rebus codebase.

[CLAassistant]

All committers have signed the CLA.

[mookid8000]

Super! 👍 One thing though: I think the purpose of the TTL would be more clear, if it was added to the .SetAutoDelete(...) method signature.

It could look like this:
public RabbitMqQueueOptionsBuilder SetAutoDelete(bool autoDelete, long ttlInMs = 0)

What do you think?

[ronnyek]

I think personally think that's more than reasonable. I only kept them apart because I didnt know if there were any major fundamental differences between the two things. RabbitMQ documentation does state the two options can be used in conjunction with one another, I just assumed if you were setting a ttl, the queue deletion was kind of implied.

Just to be clear, are you suggesting combining setAutoDelete and setTtl correct?

I'll get the pull request updated with the suggestions there. Thanks!

[ronnyek]

One question I had about this, I'm depending on Arguments collection not being overwritten with SetArguments later. Should set arguments be something more of update/insert instead of just re-assigning entire arguments dictionary?

[mookid8000]

Should set arguments be something more of update/insert instead of just re-assigning entire arguments dictionary?

Yes, I think that would cause the least surprises.

[mookid8000]

are you suggesting combining setAutoDelete and setTtl correct

Yes 😄

[ronnyek]

I went ahead and submitted the changes for the signature combination. I did not include the changes for the setArguments as I think simply setting/overriding settings in arguments would work, but potentially leave you no way to null out values, remove values all together....

I considered something like the following, but wanted your take on it before submitting that yet. I can create another pull request.

public RabbitMqQueueOptionsBuilder SetArguments(Dictionary<string, object> arguments)
        {
            if(arguments != null)
            {
               arguments.ForEach(kvp => {
                  Arguments[kvp.Key] = kvp.Value;
               });
            } else {
               Arguments.Clear();
            }
            return this;
        }

or maybe a a parameter that explicitly clears out the arguments dictionary prior to setting the values.

[mookid8000]

excellent, thanks 😄 it'll be out in Rebus.RabbitMq 5.1.0 in a few minutes

[ronnyek]

I don't mean to continue to beat this one to death, but I've just recently noticed that it SEEMS like rabbit might treat those properties (autodelete and x-expires) as different properties. I've got autodelete set to true, and passing in 5 days in milliseconds, and as soon as last consumer disconnects (not obeying the queue deletion time), the queue gets deleted.

I'm reaching out to people in rabbitmq community, hoping for comments on that (since the documentation on their site is.... not very thorough in this specific dept. I want to figure it out and fix and make sure to get that pull requested back in.

[mookid8000]

Damn!! Thanks for taking the time to report your findings in here!

[ronnyek]

So, it SOUNDS like to me that the features are not tied together... autodelete is just delete after all consumers disconnect... and x-expires is expire the queue after the expiration time is met. I'm thinking SetAutoDelete(bool autodelete, long ttlInMs) need to be split apart.

I was thinking about the idea of making it SetAutoDelete(bool) and SetQueueExpiration(long ttlInMs) just so intentions are more explicit, and documentation could be clearer.

Thoughts? Comments?