-
Notifications
You must be signed in to change notification settings - Fork 43
Restart closed queues #1345
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
base: main
Are you sure you want to change the base?
Restart closed queues #1345
Conversation
|
Claude finished @viktorerlingsson's task —— View job Code Review CompleteI've reviewed the PR for bugs, security vulnerabilities, performance issues, missing error handling, and Crystal anti-patterns. Issues Found:1. Missing Return Type Annotation (src/lavinmq/amqp/queue/queue.cr:208)
2. Potential State Inconsistency (src/lavinmq/amqp/queue/queue.cr:222)
3. Missing Error Context (src/lavinmq/http/controller/queues.cr:124)
Todo List:
The implementation is generally solid with good test coverage. The restart functionality correctly handles message store recreation and channel reinitialization. No security vulnerabilities or performance issues identified. |
e68911b to
9bcec6b
Compare
|
Are the claude code review comments valid? |
9bcec6b to
3e5c40a
Compare
Some of them where, some not! I've addressed the ones I think was relevant |
3e5c40a to
f309b45
Compare
snichme
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do feel that this feature is valuable to have I'm just concerned if doing adds more complexity to the queue code that will be hard to handle in the future.
| start | ||
| end | ||
|
|
||
| private def reset_queue_state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that we already have a lot of state to keep track of, while this doesn't add more states it adds more places where we need to keep track of all the states for a queue.
Not sure if there is a better solution for this at the moment. I just wanted to flag that the more state changes we add to more difficult it will be to keep everything in correct state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, there is a lot to keep track of. I guess another option would be to delete the queue object in VHost and recreate it, but I think that just moves the complexity to VHost instead.
snichme
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did another round on this, found some more things to consider
| with_vhost(context, params) do |vhost| | ||
| refuse_unless_management(context, user(context), vhost) | ||
| q = find_queue(context, params, vhost) | ||
| q.restart! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
restart! is a no-up, just returns when the queue is running ( unless @closed)
Maybe recover would be a better name?
And also, have a response that say that the queue was restarted/recovered so you dont issue a PUT to this endpoint and you get nothing back but also nothing happens with the queue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah , I took inspiration from pause! / resume!. But we can make it a little smarter so the user can know what happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I write the initial implementation on pause and resume, so even I should learn from my own comments.
But that is for another PR :)
WHAT is this pull request doing?
This adds functionality to restart closed queues/streams.
Adds:
Fixes #1290
HOW can this pull request be tested?
Run the new specs