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

Management UI: extend Get Message feature to streams #11030

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LoisSotoLopez
Copy link
Contributor

Proposed Changes

Extends the management endpoint for retrieving messages from a queue allowing this operation to be applied to stream queues. Alters the queue.ejs template to show the Get messages box when showing a stream queue information.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)
  • Build system and/or CI

Checklist

Put an x in the boxes that apply.
You can also fill these out after creating the PR.
If you're unsure about any of them, don't hesitate to ask on the mailing list.
We're here to help!
This is simply a reminder of what we are going to look for before merging your code.

Further Comments

@@ -7,6 +7,7 @@

-module(rabbit_mgmt_wm_queue_get).

-include_lib("kernel/include/logger.hrl").
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this used anywhere? or just a leftover from debugging?

ReqData, Context)
end)
end).
Resource = rabbit_misc:r(<<"/">>, queue, Q),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Resource = rabbit_misc:r(<<"/">>, queue, Q),
Resource = rabbit_misc:r(VHost, queue, Q),

{redelivered, Redelivered},
{exchange, Exchange},
{routing_key, RoutingKey},
{consumer_tag, ConsumerTag},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consumer tag is just hardcoded "ctag" so it does not need to be included.

delivery_tag = DeliveryTag,
consumer_tag = ConsumerTag},
#amqp_msg{props = Props, payload = Payload}} ->
ok = amqp_channel:cast(Ch, #'basic.ack'{delivery_tag = DeliveryTag,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think we want to ack the message. If the stream has 20 messages, and we ask for 10 (prefetch = 10) when we ack the first message, the broker will send the 11th message. (Prefetch count means the max number of outstanding ie unacknowledged messages)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feature should not arguably use AMQP 0-9-1 at all. And the original feature should never have used an AMQP 0-9-1 Erlang client to begin with.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it would probably be better to use rabbit_queue_type directly to get a message off the stream.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont say that an amqp connection should be used to implement this feature, just want to mention the aspect, or implicit benefit that the server side of an amqp connection does additional authorization checks.

Would like to cite the recent attempt for the delete queue endpoint to use an internal API #9550 which was reverted #10062 (comment). Maybe can you recall what was the difficulty, why the path outlined in #10062 was abandoned?

Or maybe that restriction is not applicable for a simple reading from a queue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a way to not move anywhere.. why would not using the 0-9-1 protocol be ok expect it's maybe not 100% optimal? This solves a real world problem with minimal change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dear Core Team! Do you know the difficulties of or things to consider when converting the Management plugin to not use an AMQP client, eg in context of #9550 and #10062? This would be helpful to move forward with this PR either using an amqp client or not.

amqp_channel:subscribe(
Ch,
#'basic.consume'{queue = Queue,
no_ack = NoAck,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NoAck is always false so we can hardcode it I think
Also hardcode the CTag

@michaelklishin
Copy link
Member

michaelklishin commented Apr 19, 2024

Before you spend any more time on this feature, have you considered consulting with the core team as to whether this feature has any chance of being accepted? Or, perhaps, whether it should use the Erlang AMQP 0-9-1 client? Or any client at all and not an internal API?

I'd very much like to see contributors, even those who contribute mere 1-2% of R&D despite directly making money off of RabbitMQ, to spend their time on more fundamental issues like #11038 or #10763 (two examples of much more fundamental problems it does not take that deep an understanding of RabbitMQ internals to work on).

Specifically around streams, rabbitmq/rabbitmq-website#1885 is infinitely more important for stream adoption than this feature.

@michaelklishin michaelklishin changed the title Add get message feat on stream queues UI Management UI: extend Get Message feature to streams Apr 19, 2024
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

Successfully merging this pull request may close these issues.

5 participants