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

Consumer arg support for shovels #578

Merged
merged 9 commits into from
Oct 30, 2023
Merged

Conversation

viktorerlingsson
Copy link
Member

@viktorerlingsson viktorerlingsson commented Sep 29, 2023

WHAT is this pull request doing?

Shovels handle src-consumer-args to enable them to handle stream queues.

HOW can this pull request be tested?

Create a shovel for a stream queue via API with x-stream-offset

@viktorerlingsson viktorerlingsson force-pushed the migrate_from_rabbit_fixes branch 2 times, most recently from 4926557 to 23b30c1 Compare October 18, 2023 12:26
@viktorerlingsson viktorerlingsson marked this pull request as ready for review October 18, 2023 12:27
@@ -41,7 +41,8 @@ module LavinMQ

def initialize(@name : String, @uri : URI, @queue : String?, @exchange : String? = nil,
@exchange_key : String? = nil,
@delete_after = DEFAULT_DELETE_AFTER, @prefetch = DEFAULT_PREFETCH, @ack_mode = DEFAULT_ACK_MODE,
@delete_after = DEFAULT_DELETE_AFTER, @prefetch = DEFAULT_PREFETCH,
@ack_mode = DEFAULT_ACK_MODE, @consumer_args = {} of String => JSON::Any || nil,
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to have @consumer_args as a instance variable? Looks like it's only used to populate @args?

And the || nil part seems odd. Will it ever do anything? Default value is empty hash. Or do you want @consumer_args : Hash(String, JSON::Any)? = nil?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, looks much better that way 👍

@@ -60,6 +61,10 @@ module LavinMQ
if @queue.nil? && @exchange.nil?
raise ArgumentError.new("Shovel source requires a queue or an exchange")
end
@args = AMQ::Protocol::Table.new
@consumer_args.try &.each do |k, v|
@args[k] = v
Copy link
Member

Choose a reason for hiding this comment

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

How will JSON::Any be handled from here? Should it be v.as_s? or something?

Copy link
Member Author

@viktorerlingsson viktorerlingsson Oct 18, 2023

Choose a reason for hiding this comment

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

I don't think v.as_s? is needed for it to work, but it's probably best to be specific

Copy link
Member

@spuun spuun Oct 19, 2023

Choose a reason for hiding this comment

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

Oh, AMQ::Protocol::Table accepts JSON::Any as value. Didn't know that (and I guess it compiled for you 🤦). It should be fine then!

@viktorerlingsson viktorerlingsson changed the title Fixes to make migration from other cluster easier Consumer arg support for shovels Oct 18, 2023
@viktorerlingsson
Copy link
Member Author

Moved the definitions upload stuff to a separate PR to keep this PR more focused

@viktorerlingsson viktorerlingsson force-pushed the migrate_from_rabbit_fixes branch from 40044d2 to 3b3b9df Compare October 30, 2023 09:40
@viktorerlingsson viktorerlingsson merged commit 219b9c5 into main Oct 30, 2023
7 of 18 checks passed
@viktorerlingsson viktorerlingsson deleted the migrate_from_rabbit_fixes branch October 30, 2023 11:08
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.

3 participants