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

Make worker_monitor_drb act like a reader again! #14638

Conversation

jrafanie
Copy link
Member

@jrafanie jrafanie commented Apr 4, 2017

https://bugzilla.redhat.com/show_bug.cgi?id=1438935

For testing in e21d1b9, @worker_monitor_drb was changed to use the
method. Unfortunately, this method isn't a reader, so it would create a
new drb client each time it was called.

It doesn't make sense for this method to make a new drb client each
time, so we can cache this client on first access so it functions like a
attr_reader now. We can then remove some memoization/caching that
occurs in heartbeat since the getter method handles that for us.

Use 147aa6b?w=1 to ignore whitespace.

Note, the parent PR, #14365, was backported to euwe.

Copy link
Member

@carbonin carbonin left a comment

Choose a reason for hiding this comment

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

You can probably also remove

@worker_monitor_drb ||= worker_monitor_drb

Otherwise looks good 👍

https://bugzilla.redhat.com/show_bug.cgi?id=1438935

For testing in e21d1b9, @worker_monitor_drb was changed to use the
method.  Unfortunately, this method isn't a reader, so it would create a
new drb client each time it was called.

It doesn't make sense for this method to make a new drb client each
time, so we can cache this client on first access so it functions like a
attr_reader now.  We can then remove some memoization/caching that
occurs in heartbeat since the getter method handles that for us.
@jrafanie jrafanie force-pushed the fix_worker_monitor_drb_new_client_each_time branch from 147aa6b to a9726db Compare April 4, 2017 20:09
@miq-bot
Copy link
Member

miq-bot commented Apr 4, 2017

Checked commit jrafanie@a9726db with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 1 offense detected

app/models/miq_worker/runner.rb

Copy link
Member

@carbonin carbonin left a comment

Choose a reason for hiding this comment

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

👍 On less caching

Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@gtanzillo gtanzillo added this to the Sprint 58 Ending Apr 10, 2017 milestone Apr 4, 2017
@gtanzillo gtanzillo merged commit 56ee45a into ManageIQ:master Apr 4, 2017
@jrafanie jrafanie deleted the fix_worker_monitor_drb_new_client_each_time branch April 5, 2017 18:07
@jrafanie
Copy link
Member Author

jrafanie commented Apr 5, 2017

@gtanzillo Making this a blocker since it floods the logs. Additionally, it creates a new DRb client each time we process a queue message, so it needs this PR fix.

simaishi pushed a commit that referenced this pull request Apr 5, 2017
…ient_each_time

Make worker_monitor_drb act like a reader again!
(cherry picked from commit 56ee45a)

https://bugzilla.redhat.com/show_bug.cgi?id=1439303
@simaishi
Copy link
Contributor

simaishi commented Apr 5, 2017

Fine backport details:

$ git log -1
commit 230d741c94cfdb0b11ecec67f956e6af424ab16a
Author: Gregg Tanzillo <[email protected]>
Date:   Tue Apr 4 18:24:33 2017 -0400

    Merge pull request #14638 from jrafanie/fix_worker_monitor_drb_new_client_each_time
    
    Make worker_monitor_drb act like a reader again!
    (cherry picked from commit 56ee45a63c780fff16722f91ba09d6acc0642ca0)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1439303

simaishi pushed a commit that referenced this pull request Apr 6, 2017
…ient_each_time

Make worker_monitor_drb act like a reader again!
(cherry picked from commit 56ee45a)

https://bugzilla.redhat.com/show_bug.cgi?id=1439308
@simaishi
Copy link
Contributor

simaishi commented Apr 6, 2017

Euwe backport details:

$ git log -1
commit 0a1ad0e0a0d12c6662e420322cc92d2d4be83c82
Author: Gregg Tanzillo <[email protected]>
Date:   Tue Apr 4 18:24:33 2017 -0400

    Merge pull request #14638 from jrafanie/fix_worker_monitor_drb_new_client_each_time
    
    Make worker_monitor_drb act like a reader again!
    (cherry picked from commit 56ee45a63c780fff16722f91ba09d6acc0642ca0)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1439308

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.

6 participants