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

Add dequeue method check to add_queue #22728

Merged
merged 1 commit into from
Oct 4, 2023

Conversation

nasark
Copy link
Member

@nasark nasark commented Oct 3, 2023

We need to be checking which dequeue method is being used when queueing events. This is because we don't want all events to be processed by the messaging client (kafka) and only when needed i.e. event syndication.

@miq-bot assign @agrare
@miq-bot add_label bug, quinteros/yes?

@@ -104,7 +104,7 @@ def self.bottleneck_event_groups
end

def self.add_queue(meth, ems_id, event)
if MiqQueue.messaging_client('event_handler').present?
if MiqQueue.messaging_client('event_handler').present? && MiqQueueWorkerBase.worker_settings[:dequeue_method] != "drb"
Copy link
Member

Choose a reason for hiding this comment

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

Since we care about the event handler specifically

Suggested change
if MiqQueue.messaging_client('event_handler').present? && MiqQueueWorkerBase.worker_settings[:dequeue_method] != "drb"
if MiqQueue.messaging_client('event_handler').present? && MiqEventHandler.worker_settings[:dequeue_method] != "drb"

@nasark nasark force-pushed the dequeue_messaging_check branch from 85473b5 to 104a16b Compare October 3, 2023 20:45
@agrare
Copy link
Member

agrare commented Oct 3, 2023

This looks good, I wonder if (follow-up) we should expose this via a class method on the MiqQueueWorkerBase class since the Runner dequeue_method_via_miq_messaging? method is basically

self.class.worker_settings[:dequeue_method] == :miq_messaging && MiqQueue.messaging_type != "miq_queue" && MiqQueue.messaging_client(self.class.name).present?`

which is essentially the same logic.

@nasark
Copy link
Member Author

nasark commented Oct 3, 2023

This looks good, I wonder if (follow-up) we should expose this via a class method on the MiqQueueWorkerBase class since the Runner dequeue_method_via_miq_messaging? method is basically

self.class.worker_settings[:dequeue_method] == :miq_messaging && MiqQueue.messaging_type != "miq_queue" && MiqQueue.messaging_client(self.class.name).present?`
which is essentially the same logic.

Ah yeah that was my initial approach but I wasn't sure if exposing the runner methods was a direction we wanted to go, I'll take note for a follow-up

@agrare
Copy link
Member

agrare commented Oct 3, 2023

Ah yeah that was my initial approach but I wasn't sure if exposing the runner methods was a direction we wanted to go

Yeah we won't be able to get to the Runner instance from here, but both should be able to see the worker class

@nasark nasark closed this Oct 4, 2023
@nasark nasark reopened this Oct 4, 2023
@nasark nasark force-pushed the dequeue_messaging_check branch from 104a16b to f6987ff Compare October 4, 2023 15:44
@nasark nasark requested a review from Fryguy as a code owner October 4, 2023 15:44
@nasark
Copy link
Member Author

nasark commented Oct 4, 2023

To pass specs, added dequeue method settings stub

Comment on lines 124 to 125
before { stub_settings_merge(:messaging => {:type => 'artemis'}) }
before { stub_settings_merge(:workers => {:worker_base => {:queue_worker_base => {:defaults => {:dequeue_method => "miq_messaging"}}}}) }
Copy link
Member

@agrare agrare Oct 4, 2023

Choose a reason for hiding this comment

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

Actually sorry I missed this wasn't a - on the first line, better to pass both keys to the stub_settings_merge

Suggested change
before { stub_settings_merge(:messaging => {:type => 'artemis'}) }
before { stub_settings_merge(:workers => {:worker_base => {:queue_worker_base => {:defaults => {:dequeue_method => "miq_messaging"}}}}) }
before do
stub_settings_merge(
:messaging => {:type => 'artemis'}),
:workers => {:worker_base => {:queue_worker_base => {:defaults => {:dequeue_method => "miq_messaging"}}}}
)
end

Copy link
Member

Choose a reason for hiding this comment

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

@nasark given the bug we had I think we should add a context here for messaging_type=>[artemis/kafka] but dequeue_method=>drb and add a test to confirm we use miq_queue not manageiq_messaging

@@ -142,6 +143,7 @@

context "messaging_type: kafka" do
before { stub_settings_merge(:messaging => {:type => 'kafka'}) }
before { stub_settings_merge(:workers => {:worker_base => {:queue_worker_base => {:defaults => {:dequeue_method => "miq_messaging"}}}}) }
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@nasark nasark force-pushed the dequeue_messaging_check branch from f6987ff to ef42db7 Compare October 4, 2023 17:25
@nasark
Copy link
Member Author

nasark commented Oct 4, 2023

Added specs and reordered conditional so we don't needlessly create a messaging client object

@nasark nasark force-pushed the dequeue_messaging_check branch from ef42db7 to da55e4a Compare October 4, 2023 17:27
@miq-bot
Copy link
Member

miq-bot commented Oct 4, 2023

Checked commit nasark@da55e4a with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@agrare agrare merged commit c7e045b into ManageIQ:master Oct 4, 2023
bdunne pushed a commit that referenced this pull request Oct 4, 2023
Add dequeue method check to add_queue

(cherry picked from commit c7e045b)
@bdunne
Copy link
Member

bdunne commented Oct 4, 2023

Backported to quinteros in 1ee2795

commit 1ee2795c82ac4ad0ee4b5b06d48c3a74cb479927 (HEAD -> quinteros, upstream/quinteros)
Author: Adam Grare <[email protected]>
Date:   Wed Oct 4 13:58:23 2023 -0400

    Merge pull request #22728 from nasark/dequeue_messaging_check
    
    Add dequeue method check to add_queue
    
    (cherry picked from commit c7e045b629d65506e5c6e6b1d9d1bce419798e22)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants