Bug 680167

Summary: RefreshableView refresh() bug - group config view shows two duplicate config editors
Product: [Other] RHQ Project Reporter: John Mazzitelli <mazz>
Component: Core UIAssignee: John Mazzitelli <mazz>
Status: CLOSED CURRENTRELEASE QA Contact: Mike Foley <mfoley>
Severity: urgent Docs Contact:
Priority: urgent    
Version: 4.0.0.B02CC: ccrouch, jshaughn
Target Milestone: ---   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-09-03 13:01:06 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On:    
Bug Blocks: 585306    

Description John Mazzitelli 2011-02-24 09:20:14 EST
go view a group's current configuration tab. you will see two config editors instead of just one.
Comment 1 John Mazzitelli 2011-02-25 09:21:39 EST
researching this issue, I may have uncovered a problem that is cross-cutting across many views - anything that implements RHQ's RefreshableView interface.

This particular class (org.rhq.enterprise.gui.coregui.client.inventory.groups.detail.configuration.GroupResourceConfigurationEditView) implements refresh() which loads the config def/config data into an editor - this is the RefreshableView interface method that gets called by our tab/subtab framework.

However, this class also calls refresh() within onDraw() explicitly. As I step through the code, I see refresh() getting called twice pretty quickly in succession (presumably one from onDraw() and once from the tab/subtab infrastructure). This means:

a) we are doing twice the work - issuing double sets of queries - just to initially load the page. Makes the page slower and makes the server do twice the work

b) due to the async nature of the server calls, the onSuccess methods are invoked twice, creating duplicate editors and adding them both as members to the parent layout (thus the two we see on the screen).

I looked at other views that implement RefreshableView and I see the same pattern repeated (that is, refresh() getting called explicitly in onDraw). Therefore, at a minimum, I would expect some of these other views are also doing twice the amount of work it should if not showing duplicate ui components on the screen.

We need to examine ALL implementations of RefreshableView and make sure we aren't making the same mistake.

Raising this to urgent since this bug could be a major performance hit across many pages.
Comment 2 John Mazzitelli 2011-02-25 10:02:26 EST
commit adbe97386a3046eef0a1e0d79765ca73050e7ecc

This commit illustrates how to fix this RefreshableView bug for those views that have this bug. Just track whether you are currently refreshing, if you are, abort the current call to refresh().

1) add a private boolean member to track the state of the refresh:

     private boolean refreshing = false;

2) in the RefreshableView.refresh() method impl, return immediately without doing anything if you are currently in the middle of a refresh. Otherwise, set the refresh flag to true and continue on the refresh logic:
 
     @Override
     public void refresh() {
+        if (this.refreshing) {
+            return; // already in the process of refreshing, don't do it again
+        }
+
+        this.refreshing = true;
         // ...
         // continue the refresh method
         // ...
     }

3) when you know the refresh is done and the UI has been updated with the refreshed data, flip the refreshing flag back to false thus enabling you to refresh it again later:

     this.refreshing = false; // we know we are done the refresh now
Comment 3 John Mazzitelli 2011-02-25 10:16:47 EST
RefreshableView.refresh() implementations:

org.rhq.enterprise.gui.coregui.client.components.table.Table
org.rhq.enterprise.gui.coregui.client.inventory.common.detail.summary.AbstractActivityView
org.rhq.enterprise.gui.coregui.client.inventory.groups.detail.configuration.GroupResourceConfigurationEditView
org.rhq.enterprise.gui.coregui.client.inventory.groups.detail.inventory.CurrentGroupPluginConfigurationView
org.rhq.enterprise.gui.coregui.client.inventory.groups.detail.summary.ActivityView
org.rhq.enterprise.gui.coregui.client.inventory.resource.detail.configuration.ResourceConfigurationEditView
org.rhq.enterprise.gui.coregui.client.inventory.resource.detail.inventory.PluginConfigurationEditView
org.rhq.enterprise.gui.coregui.client.inventory.resource.detail.inventory.ResourceResourceAgentView
org.rhq.enterprise.gui.coregui.client.RefreshableView
Comment 4 John Mazzitelli 2011-02-26 00:38:18 EST
i fixed all the problems classes from the list commented earlier.

however, I also see this problem in PluginConfigurationHistoryView (resource connection settings history) which means its probably also in ConfigurationHistoryView (resource config history)

Use "control-shift-s" stats window to see the server calls being made - and you can see when you switch back to the plugin config history subtab, it calls getPluginConfig and getLastedPluginConfig twice each (should only do it once)
Comment 5 John Mazzitelli 2011-04-12 14:47:21 EDT
Here's the problem Jay S. and I found while looking at the config history view problem as described in comment #4 :

org.rhq.enterprise.gui.coregui.client.util.preferences.UserPreferences.setAutomaticPersistence(boolean)

is calling CoreGUI.refresh() which is forcing the second table refresh/query. We need to be careful where CoreGUI.refresh() is getting called.

The issue here is when you change to a different resource, we add that new resource to the "Recently Viewed" user preference setting. But changing user preferences force an automatic update to the database (we store the new resource to the recently viewed preference). This in itself is not bad. However, because this forces an automatic storage of the preference, that setAutomaticPersistence's inner anonymous callback class is called and it calls refresh().

We do still need to refresh on some changes to preferences, such as changing the metric range preference (because we need to re-query for data in the new date range).

Somehow we need that auto-persistence callback to only refresh when certain preferences change, but it should not blindly refresh on all changes to all preferences.
Comment 6 John Mazzitelli 2011-04-13 09:56:31 EDT
commit 87b488f907b471667c40fe412baad5aecf8042a4 added a fix to the issue in comment #5 but I'm still seeing duplicate queries. Still hunting for more problems in this area.
Comment 7 John Mazzitelli 2011-04-13 10:59:36 EDT
commit abc7cd665aa8a4129ad25f26371fe8409f995dc7 is a fix ips made to TableSection to try to fix some refresh issues as well
Comment 8 John Mazzitelli 2011-04-13 15:48:33 EDT
re: comment #7 and ian's fix - that didn't address this issue. so ignore that comment.
Comment 9 Jay Shaughnessy 2011-04-13 16:12:06 EDT
This change hopefully puts an end to the duplicate queries. The issue in config history was a separate issue from the other fixes, not related to refresh.

commit ea2dc1196a085b63e13ed159ba82cc089cb451ce
Author: Jay Shaughnessy <jshaughn@redhat.com>
Date:   Wed Apr 13 15:26:53 2011 -0400

    - Prevent listgrid sort request from generating an unwanted config
      history datasource fetch.  Utilize server-side sorting via PageList
      support in Criteria API.
Comment 10 John Mazzitelli 2011-04-13 16:26:46 EDT
commit d3b8e692977850ad7469c6c6e8a423f8ff0f58a8 adds the same PageControl sorting to the plugin config history view
Comment 11 John Mazzitelli 2011-04-13 16:30:09 EDT
all the issues that were known are now fixed. not sure if/how QA can test. You have to know what queries need to be issues to know if the number of queries is doubled. Using the control-shift-s or control-alt-s stats windows will show you how many requests were sent to the server.

In the last case we fixed, go to the Config>History tab of the RHQ Agent resource. Then go to the RHQ Agent child "rhq-agent-env.sh" (remaining on the config>history tab) and you should only see one query to getLiveConfiguration and getResourceConfigurationUpdatehistory. That's one example of the kind of test to do.

I'll let QA determine if they want to setup tests for this one or not.
Comment 12 Mike Foley 2011-05-24 09:14:40 EDT
verified through smoke-testing Group-Config.  This was qualified on Master, as well as the 4.0.1 community release branch.
Comment 13 Heiko W. Rupp 2013-09-03 13:01:06 EDT
Bulk closing of old issues that are in VERIFIED state.