Bug 680167 - RefreshableView refresh() bug - group config view shows two duplicate config editors
Summary: RefreshableView refresh() bug - group config view shows two duplicate config ...
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: RHQ Project
Classification: Other
Component: Core UI
Version: 4.0.0.B02
Hardware: Unspecified
OS: Unspecified
urgent
urgent
Target Milestone: ---
: ---
Assignee: John Mazzitelli
QA Contact: Mike Foley
URL:
Whiteboard:
Depends On:
Blocks: rhq4
TreeView+ depends on / blocked
 
Reported: 2011-02-24 14:20 UTC by John Mazzitelli
Modified: 2013-09-03 17:01 UTC (History)
2 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2013-09-03 17:01:06 UTC
Embargoed:


Attachments (Terms of Use)

Description John Mazzitelli 2011-02-24 14:20:14 UTC
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 14:21:39 UTC
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 15:02:26 UTC
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 15:16:47 UTC
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 05:38:18 UTC
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 18:47:21 UTC
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 13:56:31 UTC
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 14:59:36 UTC
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 19:48:33 UTC
re: comment #7 and ian's fix - that didn't address this issue. so ignore that comment.

Comment 9 Jay Shaughnessy 2011-04-13 20:12:06 UTC
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>
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 20:26:46 UTC
commit d3b8e692977850ad7469c6c6e8a423f8ff0f58a8 adds the same PageControl sorting to the plugin config history view

Comment 11 John Mazzitelli 2011-04-13 20:30:09 UTC
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 13:14:40 UTC
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 17:01:06 UTC
Bulk closing of old issues that are in VERIFIED state.


Note You need to log in before you can comment on or make changes to this bug.