Runnable / hermes

Runnable job queuing system interface abstraction for runnable's services
MIT License
3 stars 1 forks source link

Add default option queue ttl 2 min #30

Closed cflynn07 closed 8 years ago

cflynn07 commented 8 years ago
anandkumarpatel commented 8 years ago

needs to be configurable, either process.env or passed in.

podviaznikov commented 8 years ago

I don't think we want to make it global for all queues. IMHO I'd prefer it to be setting per queue

cflynn07 commented 8 years ago

@podviaznikov I'd like to see it configurable per queue as well. But that would mean we'd have to pass in a slightly more complex data structure to hermes to associate queue settings with queue names. I'd like to make that change after getting basic TTL functionality

podviaznikov commented 8 years ago

Yes, but implementing it with "more complex data structure" will take ~1 hour. Why do it this way, update your code to use new version of hermes, then make proper implementation and update your code that uses hermes 1 more time

cflynn07 commented 8 years ago

I think the process will be a little more involved that may immediately be apparent due to all the various current&near-future uses of hermes throughout each of our services, but I understand the benefits of more granular configurability. I'll use a fork of hermes with ttl settings to ship my latest feature that requires it, and I'll revisit this task when I have more time to plan it

bkendall commented 8 years ago

This is a queue TTL? How does a durable queue work with a TTL?

anandkumarpatel commented 8 years ago

add unit test please

cflynn07 commented 8 years ago

@bkendall The documentation with all the details: http://www.rabbitmq.com/ttl.html#queue-ttl

anandkumarpatel commented 8 years ago

+1 from me, get 1 more person to approve

podviaznikov commented 8 years ago

still super strange that proper implementation with possibility to configure ttl per queue wasn't implemented (that means we would need to spend more time reviewing another small pr in the future, updating navi etc) but +1.