Dont write all binds to defs #570
Conversation
|
We should probably add some kind of occasional compactation during runtime. Count "deletes" and trigger compaction after X deletions or something, so prevent the file from growing on e.g. high queue churn. |
Sounds good! |
| ret = @queue_bindings[{routing_key, nil}].add? destination | ||
| after_bind(destination, routing_key, headers) | ||
| ret |
There was a problem hiding this comment.
Should after_bind be executed even if the binding was already there?
| ret = @queue_bindings[{routing_key, nil}].add? destination | |
| after_bind(destination, routing_key, headers) | |
| ret | |
| if @queue_bindings[{routing_key, nil}].add? destination | |
| after_bind(destination, routing_key, headers) | |
| true | |
| end |
There was a problem hiding this comment.
Maybe, I just didn't want to change the behavior. Seems to be federation link that listens for bind/unbind events to forward them to the upstream.
src/lavinmq/vhost.cr
Outdated
| return false unless src.bind(dst, f.routing_key, f.arguments.to_h) | ||
| store_definition(f) if !loading && src.durable && dst.durable |
There was a problem hiding this comment.
Doesn't false indicate that the client will get a PreconditionFailed back? as in the case above if the exchange is missing? Should we just not store the defintion instead?
| return false unless src.bind(dst, f.routing_key, f.arguments.to_h) | |
| store_definition(f) if !loading && src.durable && dst.durable | |
| if src.bind(dst, f.routing_key, f.arguments.to_h) | |
| store_definition(f) if !loading && src.durable && dst.durable | |
| end |
There was a problem hiding this comment.
Maybe :) It wasn't obvious on what the return value meant (or how it's used). At least not used in Client. Maybe we should use enums or symbols here and there instead to get more descriptive return values?
There was a problem hiding this comment.
I think i chose to return false because we did so for Queue::Declare if the queue already existed.
- No need to add an existing binding - No need to unbind a non-existing binding Without this guard the definitions.amqp file grows indefinitely
Verify that bind/unbind frames aren't written to definition files unless needed.
Instead of adding `has_binding?` which adds some unecessary `.to_h` `bind`/`unbind` will return a boolean indicatin whether the binding has been added/deleted or not.
carlhoerberg
force-pushed
the
dont-write-all-binds-to-defs
branch
from
f9d3ae7 to
75ecbbd
Compare
WHAT is this pull request doing?
definitions.amqp can grow indefinitely because we write all bind and unbind frames to it. This will add guard against that behavior.
HOW can this pull request be tested?
Add the same binding multiple times and verify it's not written to definitions.amqp. If log level is debug it will be logged if written to file.