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

Clean out cache of MiqProductFeature at CUD tenant actions in UI process #5108

Conversation

lpichler
Copy link
Contributor

@lpichler lpichler commented Dec 20, 2018

CUD - create, update, delete

Issue
MiqProductFeature is using cache for some queries. MiqProductFeatures are tied to Tenants. Where we are adding/updating/delete tenant we need update this cache to reflect changes in UI.
But it doesn't work when cache is cleaned out at model's callbacks because UI is different process.

Reproducer
https://user-images.githubusercontent.com/14937244/50166079-8c3c3380-02e6-11e9-8a53-a39dddcb9cab.gif

Thanks to the @gtanzillo's idea and @skateman's research we confirmed that if you clean the cache from the API(on instantiated classes), it doesn't reflects the changes in the UI probably because of different processes. We found with @himdel obviously better solution how to tie cleaning of cache to CUD of tenants. (then it was presented here #5101 and #5100)

Fix tested on appliance.

@miq-bot assign @himdel

@miq-bot add_label bug, blocker

Links

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

@lpichler
Copy link
Contributor Author

lpichler commented Dec 20, 2018

as well at at many PRs this is example of great teamwork! Thank you! 👏 👏

@lpichler lpichler force-pushed the clear_cashe_of_MiqProductFeature_in_UI_process_at_CUD_tenant_actions branch 2 times, most recently from 773cd1a to 6806dc7 Compare December 20, 2018 19:47
@skateman
Copy link
Member

@lpichler it's definitely not due to different processes as I was able to reproduce the issue using rails s. Even though the solution might work, I think it is a very ugly hack to create a route just because there's something we don't completely understand. Of course if there's no other solution, we have to work with what we have.

@miq-bot add_reviewer @martinpovolny

@lpichler lpichler force-pushed the clear_cashe_of_MiqProductFeature_in_UI_process_at_CUD_tenant_actions branch from 6806dc7 to c5057e5 Compare December 20, 2018 20:15
@miq-bot
Copy link
Member

miq-bot commented Dec 20, 2018

Checked commit lpichler@c5057e5 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🏆

@lpichler
Copy link
Contributor Author

it's definitely not due to different processes as I was able to reproduce the issue using rails s.

I was not able to reproduce it with rails s.

I think it is a very ugly hack to create a route just because ...

We know about it. It is not about how fix is ugly but what is the best solution at this moment, whether it is possible to cover it by specs and if it includes features of ideal solution.

@martinpovolny martinpovolny requested review from himdel, mzazrivec and ZitaNemeckova and removed request for martinpovolny December 21, 2018 10:26
@martinpovolny
Copy link
Member

Delegating this one. I do not feel strong enough to review the Angular changes.

@skateman
Copy link
Member

@martinpovolny my problem here is not with angular, but with the principle of adding a route for having something that is clearly just a workaround, i.e. creating techdebt.

@martinpovolny
Copy link
Member

@skateman : I agree. However we need a fix for a blocker BZ.

I would consider making this a fix for hammer and carrying on with investigating the issue.

However I do not understand the problem well enough to have a strong opinion here.

@skateman
Copy link
Member

@lpichler can you please at least mark the route/implementation with a FIXME and explain in a comment that this should be revisited?

@ZitaNemeckova
Copy link
Contributor

@martinpovolny Angular LGTM 👍

@himdel himdel merged commit a385b30 into ManageIQ:master Dec 21, 2018
@himdel himdel added this to the Sprint 102 Ending Jan 7, 2019 milestone Dec 21, 2018
@lpichler lpichler deleted the clear_cashe_of_MiqProductFeature_in_UI_process_at_CUD_tenant_actions branch December 21, 2018 12:07
@lpichler
Copy link
Contributor Author

lpichler commented Jan 2, 2019

@miq-bot add_label hammer/yes

simaishi pushed a commit that referenced this pull request Jan 7, 2019
…re_in_UI_process_at_CUD_tenant_actions

Clean out cache of MiqProductFeature at CUD tenant actions in UI process

(cherry picked from commit a385b30)

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

simaishi commented Jan 7, 2019

Hammer backport details:

$ git log -1
commit a6ad477c443b23a485fcf755aadbb56d142c171d
Author: Martin Hradil <[email protected]>
Date:   Fri Dec 21 12:52:40 2018 +0100

    Merge pull request #5108 from lpichler/clear_cashe_of_MiqProductFeature_in_UI_process_at_CUD_tenant_actions
    
    Clean out cache of MiqProductFeature at CUD tenant actions in UI process
    
    (cherry picked from commit a385b30de33e1d723227ee65e1cab2ad30a0b7a4)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1468795
    https://bugzilla.redhat.com/show_bug.cgi?id=1655012

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.

7 participants