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

[frontend] Publish on the rabbitmq exchange #4027

Merged
merged 1 commit into from
Oct 17, 2017

Conversation

coolo
Copy link
Member

@coolo coolo commented Oct 16, 2017

start_connection declares the exchange, so just publish on this one instead
of creating one queue per event type.

@codecov
Copy link

codecov bot commented Oct 16, 2017

Codecov Report

Merging #4027 into master will increase coverage by <.01%.
The diff coverage is 96.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4027      +/-   ##
==========================================
+ Coverage    89.2%   89.21%   +<.01%     
==========================================
  Files         308      308              
  Lines       18200    18199       -1     
==========================================
  Hits        16236    16236              
+ Misses       1964     1963       -1
Flag Coverage Δ
#api 80.45% <96.96%> (ø) ⬆️
#rspec 69.76% <96.77%> (ø) ⬆️
#webui 61.73% <96.96%> (ø) ⬆️

@@ -3,8 +3,7 @@ def self.publish(event_queue_name, event_payload)
return unless CONFIG['amqp_options']
start_connection

queue = $rabbitmq_channel.queue(event_queue_name, CONFIG['amqp_queue_options'].try(:with_indifferent_access) || {})
$rabbitmq_exchange.publish(event_payload, routing_key: queue.name)
$rabbitmq_exchange.publish(event_payload, routing_key: event_queue_name)
Copy link
Member Author

Choose a reason for hiding this comment

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

Actually self.class.message_bus_queue is already a misnomer. We don't want different queues per event type, just different routing keys.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know that is a PITA but I would prefer to change the name if it doesn't really make sense... something like self.class.message_bus_route is better??

Copy link
Contributor

Choose a reason for hiding this comment

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

And also then changing event_queue_name to event_route ??

Copy link
Member Author

Choose a reason for hiding this comment

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

Your suggestion would be better than what we have. To quote from the net: "A message goes to the queue(s) whose binding key exactly matches the routing key of the message.". What we are passing here is that routing key - so possibly we would be most correct if we called it event_routing_key

[webui][api] Update Event classes to publish only once.
@coolo
Copy link
Member Author

coolo commented Oct 17, 2017

please re-review

@mdeniz mdeniz merged commit 7a01403 into openSUSE:master Oct 17, 2017
@coolo coolo deleted the publish_on_exchange branch October 17, 2017 11:35
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.

None yet

2 participants