all two-level tab navigation was broken
by Joseph Marques
Two-level tab navigation appeared to be bust this weekend. After some
investigation I was able to track the issue down to commit b20dc08 from
Friday/11am (fix so subtab content refreshes when you switch back to a
subtab BZ-661528). That fix altered the following line:
if (subView instanceof BookmarkableView) {
to:
if (subView instanceof BookmarkableView && !viewPath.isEnd()) {
so that the else block, which checks for RefreshableView, would
trigger. The larger effect of this, however, is that renderView for
TableSection is never invoked...not on first page load, not on tab
switching, not on subtab switching. This then has the effect that the
private member variable 'basepath' for TableSection is never set, which
means that the master/details navigation is broken (row double-click,
name-column hyperlink, back-to-list functionality, new button, etc).
So I removed the check for isEnd() and things are back to normal for
now, but it likely reintroduces some or all of BZ-661528. I've flipped
that bugzilla back to ON_DEV, and reassigned to Ian to take a look at,
but I think the bigger issue here is how easy it is to regress on a
large part of the navigation experience when plumbing around in our
infrastructure classes. We either need to put some emphasis on
automated testing in this area, or be a bit more diligent about manually
testing for regressions when committing enhancements to these classes.
-joseph
13 years, 5 months
optional criteria-based data/count fetching
by Joseph Marques
commit 8e313057f12a8ecdb379c32b70f0daa99cceb223
Author: Joseph Marques<joseph(a)redhat.com>
Date: Tue Dec 7 01:43:12 2010 -0500
strategy to allow more efficient criteria-based query generation& execution
* can now call Criteria.setRestriction(Restriction) to bypass generation
and execution of either the data-query or the count-query. this reduces
processing and eliminates one round trip to the database each time the
given criteria is used to fetch.
-----
You can now easily specify in any criteria-based SLSB method whether you want to return the page of data, the cardinality of the non-paged result (e.g., count / totalsize), or both (the classic semantics). Why is this useful? Well, take for example the "X recent alerts" message you see in the application footer. In this case, all we need is the count of the alerts generated by any resource visible to the user over the last 8 hours, so we can use the count restriction:
AlertCriteria criteria = new AlertCriteria();
criteria.addFilterStartTime(System.currentTimeMillis() - (1000L * 60 * 60 * 8));
criteria.setRestriction(Restriction.COUNT_ONLY);
GWTServiceLookup.getAlertService().findAlertsByCriteria(criteria, AsyncCallback);
Under the covers, only the count-query will be generated (ever so slightly less load on the app server) and executed (less load on the DB, especially as the complexity of the generated query increases...and one less DB roundtrip). In a similar fashion, I can specify that I only care about the data by setting Restriction.COLLECTION_ONLY, which would skip generating/executing the count-query. This would be useful if you want to:
* fetching the entire collection - assumption is a small collection where all elements should necessarily be shown, such as the list of measurement schedules for a given resource.
* selecting a specific object (such as addFilterId) - assumption is count won't be used in this case, because all you really want is the single object, such as Resource{Group}DetailView objects
* searching and restricting the result set - assumption is that you only want to show the first 20-30 results, and don't care about the count because you're not dropping the data into a table that needs to know how many total objects there are to properly render pagination controls such as a vertical scrollbar
So please use criteria restrictions where it makes sense.
13 years, 5 months
new editable summary fields
by John Mazzitelli
I think I solved the "ghost pencil" problem (the editable/read-only
fields in the summary areas where the pencil was slow to render and then
when you moved away it stuck around for 5s and then disappeared).
https://bugzilla.redhat.com/show_bug.cgi?id=662337
(note: I got started down this path because I needed an editable
boolean/checkbox field for the recursive-flag setting - see
https://bugzilla.redhat.com/show_bug.cgi?id=661887 )
I checked in a new implementation. ToggleableTextItem has gone away,
EnhancedDynamicForm was altered to no longer use that type and a new
custom RHQ form item component has been introduced - EditableFormItem,
with a subclass CheckboxEditableFormItem (the former allows you to edit
via a text field, the latter allows you to edit a boolean value via a
checkbox. EditableFormItem is extensible in case we want to add other
editable fields like radio buttons, text areas, drop down menus, etc.).
In our views that use this today, they are still in
EnhancedDynamicForms, but this new implementation no longer requires you
to use that custom enhanced dynamic form type - all you need is some
DynamicForm and you add an instance of EditableFormItem to it.
I checked viewing and editing users and roles (both as rhqadmin and a
non-admin user), I checked editing individual resource
name/description/location, and I checked editing group
name/description/recursive-flag settings - all work well.
Let me know if you see any problems.
Note that I have two outstanding issues that I document in the BZ - one
is that the size of the field isn't as wide as it could be and two, I
don't know what to do in the case of a blur. Here's the comments from
the BZ:
1) nothing happens if you lose focus from the edit field. What do we
want it to do? Cancel or Save? Right now, it does nothing - you have to
hit "enter", or click the ok or cancel icons to get it to do something.
2) the width of the field isn't entirely correct. It looks like the
width of the item isn't as wide as the dynamic form column. Need to
figure out the proper API calls to make so the item expands to fit the
entire width of the form.
13 years, 5 months
New preview release of RHQ 4 available
by Heiko W.Rupp
Hi,
we have just made a new developer preview release of RHQ available. This
release features a lot of changes and improvements in the UI, that is rewritten
in GWT.
Major new features:
• Improved LDAP support
• Default dashboard improvements
• Improved Resource Inventory support
• Breadcrumbs no longer used
• I18N
• New Help section: Docs from the www.rhq-project.org are now available directly through the app. The dashboard also links out to the site to gather links to the latest RHQ News. For information on www.rhq-project.org privacy policy, see here: http://www.rhq-project.org/display/RHQ/Privacy+Policy
• GWT-based user and role edit views finished
• Metric and Alert template views converted from JSF to GWT
• Much improved MySQL plugin (by Steve Milidge)
More detailed release notes can be found here:
http://rhq-project.org/display/RHQ/Release+Notes+4.0+DP2
Please check out this release and give us as much feedback as you can.
Also we are very interested in getting more translations
The release can be downloaded from the link within the release notes
Thanks to everyone who contributed.
Heiko (on behalf of the RHQ team)
--
Reg. Adresse: Red Hat GmbH, Otto-Hahn-Strasse 20, 85609 Dornach bei München
Handelsregister: Amtsgericht München HRB 153243
Geschaeftsführer: Brendan Lane, Charlie Peters, Michael Cunningham, Charles Cachera
13 years, 5 months
re-ordering the tabs
by John Mazzitelli
I'm looking at the tabs, and I don't see any rhyme or reason why they
are ordered the way they are:
Summary, Monitor, Inventory, Operations, Alerts, Configuration, Events
Can I recommend we change this order to:
Summary, Inventory, Alert, Monitor, Operations, Configuration, Events
for the following reasons:
Summary, Inventory relate to what makes up the resource - what it IS -
it defines the resource itself and how its constituted. So we keep them
together and up front.
Alerts relate to what the resource did or is doing that is abnormal -
its really standalone. But we have it here, third, for a reason, see below.
Monitor, Operations, Configuration, Events are all subsystems that
describe what the resource is doing or can do. Thus, we keep them all
together. Since Operations, Config and Events are optional, they appear
after Monitor (thus, since Monitor always exists, its always first in
this group, the other three can appear in any combination depending on
the resource type).
Note:
Summary, Inventory, Alert and Monitor always exist - so the first four
tabs will always be in the same place for any resource (i.e. the UI will
be consistent no matter what resource you are looking for). e.g. If I
want to go to an alert tab, I know, regardless of what resource I'm
looking at, I need to go to the third tab.
And for groups, its basically the same - the first THREE tabs are always
the same. So, no matter what I'm looking at - a compat group, mixed
group, auto-group or individual resource, the first three tabs are
always the same. Full consistency of the tabs across every view - the
only tabs that come and go are the subsystem tabs and they are on the
far right - the location of summary, inventory, alerts, monitor never move.
Unless there are objections, I would like to alter the tabs so they
appear in the order as described above.
13 years, 5 months
detailed code review for delete-agent-plugin branch
by Joseph Marques
commit eded9dd4d1657e25f2f661801ecbc2c11b12214d
Author: John Sanda <jsanda(a)redhat.com>
Date: Tue Oct 19 21:32:19 2010 -0400
Updating resource type queries to filter on the deleted flag
Since the deleted flag is new, all existing queries need to be updated
to filter on deleted = false.
* I would use "type.deleted = false" instead of "rg.resourceType.deleted
= false",
to be sure that Hibernate doesn't unnecessarily create another join
to the
rhq_resource_type table. The where clause that says "rg.resourceType
= type"
will likely be translated to "rg0.resource_type_id = rt0.id", but I'm
not convinced
Hibernate is smart enough to translate "rg.resourcetype.deleted =
false" into
"rt0.deleted = false" as opposed to joining to the rhq_resource_type
table yet again,
and then doing "rt1.deleted = false". And the same goes for the
ResourceSearchAssistant.
* Notice how are you filtering by "crt2.deleted = false" twice, once in
the outer-query
and once in the sub-query. Adding it once to the outer-query is
sufficient, as the same
table alias will be used, thus the rows will already have been
filtered for the inner
query. For "rt2.deleted = false", you can remove that check
altogether. Notice the
"rt2.category = 'PLATFORM'" filter. I don't believe that the platform
plugin can be
disabled, as it is a requirement for any other plugin to work. Even
if we don't support
native metrics for the platform, we would still fall-back to the
Java-based platform.
Thus, since we're filtering "rt2" by platforms, rt2 can never be
disabled.
-----
commit ed4487ba743804f0a7763046be44bd15402ad683
Author: John Sanda <jsanda(a)redhat.com>
Date: Wed Oct 20 11:51:59 2010 -0400
Tidying up queries based on feedback from code review
Modified GroupSearchAssistance and ResourceSearchAssistant to use
type.deleted = false instead of rg.resouceType.deleted = false to try
and avoid unnecessarily creating another join.
Removing superflous checks against deleted flag in queries in
ResourceType.java.
* each part of the union for the named native queries needs to filter by
deleted types.
it was mentioned in the previous code review that adding it once to
the outer query
was sufficient, but it was removed from BOTH the inner and outer
queries. in other
words, the named native queries are no longer filtering by the
deleted bit at all.
-----
commit 5fbc032760c132cfe2a23e0285574a89184ab91d
Author: John Sanda <jsanda(a)redhat.com>
Date: Wed Oct 20 21:19:55 2010 -0400
The find child query also needs to filter on the deleted flag for child
types
* ResourceTypeTest.assertDeletedTypesIgnored(List<ResourceType>) should
provide a better
error message upon failure, perhaps specifying the name of the type
that was not marked
as deleted to indicate to those reading the test failure messages
what went wrong.
-----
commit e045184a9d3de2e47c5c56412fdb99f26edfe903
Author: John Sanda <jsanda(a)redhat.com>
Date: Tue Oct 26 13:49:02 2010 -0400
Reject inventory reports that contain "stale" resource types
If an inventory report that is sent up to the server contains a resource
type that is marked for deletion, DiscoveryBossBean will throw an
exception letting the plugin container know that it needs to restart so
that it can update its plugins and resource types.
This is also the initial commit for the domain-test-utils module. I have
pulled ResourceBuilder and ResourceTypeBuilder out of the cli module and
moved them into this new module so that they can more easily be shared
across modules.
* DeletedResourceTypeFilter compares types using name/plugin, is that
because the
resource type id might not have been set in the report? if so,
that's fine, just wanted
confirmation there wasn't a better way to correlate these.
-----
commit 22e236aa3085f9dd1a52a574fa6d06c7d325380d
Author: John Sanda <jsanda(a)redhat.com>
Date: Wed Oct 27 08:53:51 2010 -0400
Reject incoming reports (on the server) that contain stale resource types
This commit contains two changes needed for BZ 549852. First, filtering
logic has been added in DiscoveryBossBean.mergeInventoryReport to reject
inventory reports that include any resource types that have been marked
for deletion. A report is rejected by throwing a StaleTypeException.
The second change is handling the StaleTypeException in the agent and in
the plugin container. A new listener interface, RebootRequestListener,
has been added to the PC. When InventoryManager catches a
StaleTypeException, it notifies the listener that a reboot is needed.
This listener is the agent. The agent shuts down the PC, clears out the
data directory, and then restarts the PC.
* You changed StaleTypeException to not inherit from
InvalidInventoryReportException,
but you could have left this and the exception handling still would
have worked as
long as you caught StaleTypeException first and then
InvalidInventoryReportException.
* You probably did this because you saw failures in
ResourceUpgradeFailureHandlingTest
and DiscoveryTestBean, which catch the exceptions in the reverse order.
-----
commit 0aeedb45b4c1924a25be152ae278427dc17edc95
Author: John Sanda <jsanda(a)redhat.com>
Date: Tue Nov 2 11:10:04 2010 -0400
Adding support for deleting alerts by template
When deleting a resource type, the alert definition templates for that
type need to be deleted as well. If they are not deleted, we run into a
FK constraint violation. This commit add JPQL queries and logic to
AlertManagerBean.deleteAlertsByContext to support deleting by template.
I have also updated and added tests in AlertManagerBeanTest. I have made
a significant change in the set up/tear down logic by utilizing dbunit.
By using dbunit we can more easily ensure that the relevant parts of the
db are in a known, consistent state before and after test runs. Tables
are seeded with data before each test, and then those tables are wiped
clean afterwards to avoid introducing side affects for other test
classes.
* Your queries are called QUERY_DELETE_BY_RESOURCE_TEMPLATE, shouldn't
it be
QUERY_DELETE_BY_RESOURCE_TYPE?
* Don't know why the entire test for this had to be rewritten to use
dbutils just to
add one small piece of logic to it
-----
commit 5df1dae546b82777875c4801182ec70d2459e87b
Author: John Sanda <jsanda(a)redhat.com>
Date: Tue Nov 2 13:44:32 2010 -0400
Delete alert templates when removing a resource type
Alert definitions associated with individual resources and with groups
are deleted as part of resource deletion. This commit adds support for
templates which are alert definitions asscociated with the resource
type.
I have also renamed or fixed a method name in
AlertDefinitionManagerBean, renaming purgeAlertDefinition to
purgeAlertDefinitions.
* AlertManagerBean.deleteAlertsByContext should use MANAGE_SETTINGS to
secure
the operation, since that is what is used to secure the
AlertTemplateManagerBean
* In deleteAlertTemplates you call
alertDefinitionManager.purgeUnusedAlertDefinitions()
at the end, but an initial glance at this some may think it is not
necessary because
the hourly purge job already does it. I would add a developer-level
comment about why
this method is being called here, and what the expectations are.
-----
commit ea7d97a8c1564b4bfec466652824d6c3e4e3b52b
Author: John Sanda <jsanda(a)redhat.com>
Date: Wed Nov 3 16:55:09 2010 -0400
Refactoring ResourceMetadataManagerBean to call ContentMetadataManagerBean
ContentMetadataManagerBean handles updates to package types and bundle
types. It only performs deletion of bundle types. Package types are
handling via a cascading delete on the owning resource type.
* You left the old code in ResourceMetadataManagerBean commented out
instead of removing it,
just gotta make sure any commented code is removed in the final version
-----
commit 94609e73f7514402ebf188c791683da2868a6e12
Author: John Sanda <jsanda(a)redhat.com>
Date: Thu Nov 4 13:09:49 2010 -0400
Initial commit for OperationMetadataManagerBean
The code for updating/deleting operation definitions in
ResourceMetadataManagerBean has been moved into
OperationMetadataManagerBean.
MetadataManagerTest now uses dbunit to clear the database prior to test
runs. The reason for this change is because after each test run of a
MetadataManagerTest (i.e., ContentMetadataManagerBeanTest) and prior to
its next run, the database has to be reset. Prior to this commit, this
has been accomplished by running dbsetup. That approach was too
inefficient. Now tests can be run without running dbsetup.
Lastly, fixing the annotations in MeasurementViewManagerTest. It was
using @BeforeTest and @AftierTest when I think the intent was
@BeforeMethod and @AfterMethod.
* the implementation for OperationMetadataManagerBean.missingInFirstSet
is overly
complicated. it should be possible to use standard facilities in the
collections API to
achieve this:
intersection(set1, set2) = new set().addAll(set1).retainAll(set2)
difference(set1, set2) = new
set().addAll(set1).addAll(set2).removeAll(intersection)
missingInFirstSet(set1, set2) = new
set().addAll(set2).removeAll(set1)
* // using direct hibernate query here because JPA 1.0 lacks support for
the IN clause
// where you can directly specify a collection for the parameter
value used in an IN
// clause
This surprises me. Aren't we doing exactly this in other queries today?
* In MetadataTest.xml, you clean rhq_alert_notif_log but not
rhq_alert_notification - why?
* Why leave the operation-specific metadata code in
ResourceMetadataManager commented out?
This needs to eventually be deleted in the final version
-----
commit 0696a382c227aded453b98a074eea472085ddd61
Author: John Sanda <jsanda(a)redhat.com>
Date: Thu Nov 4 17:43:39 2010 -0400
Adding more tests for operation meta data updates
* OK, I see some commented-out code is removed here
-----
commit 48138aea74550f5a45545e87fa90006728b9f970
Author: John Sanda <jsanda(a)redhat.com>
Date: Thu Nov 4 21:33:52 2010 -0400
Moving event update code into new EJB, EventMetadataManagerBean
* The impl for "missingInFirstSet" is replicated here - again, I think
these utilty
classes should be moved to CollectionUtils where they can be reused
amongst all the
metadata managers -- OK, i see you do this in a later commit.
-----
commit c0bec23328c71709c1986a9db82f1f8d80843d2a
Author: John Sanda <jsanda(a)redhat.com>
Date: Fri Nov 5 11:40:19 2010 -0400
Changing annotation from @BeforeSuite to @BeforeClass
I think the ejb look was failing in this test because having the set up
method marked with @BeforeSuite, it could run before the embedded
container is initialized.
* You use "import java.util.*" but our code conventions say that each
import must
explicitly refer to a single type and be on a separate line, i.e. no
'*' imports allowed
-----
commit d0e3bf7b40146e89d329192cf320da117301e756
Author: John Sanda <jsanda(a)redhat.com>
Date: Fri Nov 5 13:02:01 2010 -0400
Initial commit for EventMetadataManagerBeanTest
Changing more @BeforeSuite annotations to @BeforeClass. This seems to
be causing failures on the delete-agent-plugin branch but not in master.
The failures make sense to me, but I do not understand 1) why they are
not also happening in master and 2) why I haven't seen them sooner since
the use of @BeforeSuite is not a recent change. With all that said, in
each case, the test class definitely should be using either @BeforeClass
or @BeforeMethod, definitely not @BeforeSuite.
* In MeasurementMetadataManagerBean.deleteMetadata(ResourceType) you
call refresh in
a loop - why not just make a single query that gets all of the
attached measurement
definitions for some resource type. this will reduce N roundtrips to 1.
-----
commit 4172623a5977640019840e51464e5491b2604ac0
Author: John Sanda <jsanda(a)redhat.com>
Date: Fri Nov 5 13:41:04 2010 -0400
More @BeforeSuite --> @BeforeClass clean up
* Did you ever figure out why @BeforeSuite is *NOT* failing in master?
-----
commit acb2424420cb6b28b442318a116cb70037347e28
Author: John Sanda <jsanda(a)redhat.com>
Date: Fri Nov 5 13:49:25 2010 -0400
Moving common collection-related method into their own util class
The methods, missininFirst and intersection were originally defined in
ResourceMetadataManagerBean. They are however methods for working with
generic collections and really belong in a separate class. The new
CollectionsUtil class probably needs to be moved into a core module.
* Yup, totally agree.
-----
commit 445dea1e7c98443583a9486e09c7069dd1106e04
Author: John Sanda <jsanda(a)redhat.com>
Date: Mon Nov 8 13:38:49 2010 -0500
Adding more tests for MeasurementMetadataManagerBean
There have been a few tests failing in server/jar on hudson for the past
several builds. Earlier today I managed to reproduce the same failures
locally. It turned out some tests that rely on the embedded container
were failing on jndi look ups because they were running before the
container had been bootstrapped. I was able get past this by excluding
the perf tests by excluding them with <exclude> in the surefire
configuration. As far as why this worked, I am clueless. Then I had a
few more failures, involving tests trying to register mbeans that were
already registered. I put some checks in AbstractEJB3Test to skip
registering the mbeans if they are already registered.
* Did we ever get to the root cause of these failures? I see the
one-off fixes to
unregister/register the mbean and check for the non-null
schedulerService, but it
worries me that we don't know why we're doing that.
-----
commit 988bafd8d08916e30c93b4fa79d6e24a588a0401
Author: John Sanda <jsanda(a)redhat.com>
Date: Tue Nov 9 16:33:44 2010 -0500
Fixing tests that were failing due to database clean up running too soon
MetadataManager previously defined @BeforeClass and @AfterClass methods for
setting up and tearing down each plugin meta data test class. The
database was
reset in the @BeforeClass method. Turns out that while the first few test
methods of say OperationMetadataManagerBeanTest run, then before the
rest of its
test methods have executed the @BeforeClass method is executed for say
ContentMetadataManagerBeanTest which wind up clearing out the same test
data
that OperationMetadataManagerBeanTest was using. To remedy the situation, I
have made every configuration/test method in each
XXXMetadataManagerBeanTest
part of the plugin.metadata group. Then I changed the @BeforeClass and
@AfterClass methods into @BeforeGroup and @AfterGroup methods respectively,
esnuring that they only run once for the entire group.
* so by virtue of using the strategy of completely wiping out the db,
does this prevent
us from leverage testng to run our tests in parallel now? how much
slower does the
test suite execute in this branch versus master?
-----
commit 329dc60511b340793658686c485601fec1b9764b
Author: John Sanda <jsanda(a)redhat.com>
Date: Tue Nov 9 20:46:22 2010 -0500
Initial commit for PluginManagerBean and PluginManagerLocal
This is another major refactoring of ResourceMetadataManagerBean. There
was a significatn amount of code that dealt with the plugin as a single
unit as opposed to dealing with the plugin as a collection of meta data.
Examples of the former case include methods for enabling/disabling a
plugin and query methods for a Plugin. That code has been pulled out
into a new ejb, PluginManagerBean. I was very cognisant of the existing
transaction boundaries in ResourceMetadataManagerBean, I was careful to
maintain them.
* I see more "import yadda.foo.bar.*" - that is not our code convention
Also, it looks like your import ordering is incorrect. Please see
http://www.rhq-project.org/display/RHQ/Coding+Standards for more info.
* other than that, this was a very good refactoring
-----
commit b3aca656bbdd36089fa41ce227bb7e2f3e496528
Author: John Sanda <jsanda(a)redhat.com>
Date: Tue Nov 9 21:59:09 2010 -0500
Initial commit for InventoryManagerBean
InventoryManagerBean is an ejb that provides the API for performing the
in-band deletion of resource types. Initially I was going to put the
method in ResourceTypeManagerBean, but this would have created a
circular dependency between ResourceTypeManagerBean and
ResourceManagerBean, resulting in deployment errors. Unit/integration
tests will be coming in a subsequent commit.
* note: we have circular dependencies all over the place in our SLSBs.
you get around this
issue by using @IgnoreDependency to relax the requirement that one be
fully started first
* would prefer to see
ResourceTypeManagerBean.deleteResourceTypesByPlugin or something
to that effect, and get rid of InventoryManagerBean
-----
commit c7adea444304ec5e9209d1616f19a7da2a2bb358
Author: John Sanda <jsanda(a)redhat.com>
Date: Wed Nov 10 13:06:35 2010 -0500
Adding tests for InventoryManagerBean.markTypesDeleted
InventoryManagerBeanTest verifies three things:
1) deleted flag of specified resource types is set
2) deleted flag of child types of those specified types is set
3) resources of all aforementioned types are uninventoried
InventoryManagerBeanTest uses dbunit to reset and populate the db prior
to each test run.
In support of InventoryManager, I have updated
ResourceTypeCriteria to allow for fetching resources. Lastly, I'm fixing
broken plugin descriptors for tests that I consumed during my last merge
from master.
* fetchResources may be dangerous if there are many instances of that
resource type
committed into inventory. since you only need the resource ids for
the given
resource types, i would suggest only fetching the resource ids (in
whichever way
you would prefer to do that - hard coded query, criteria query,
etc). in other words,
don't fetch more data than you need across the line.
-----
commit 8c091d0f5665967af31bd2fab23e62c4edc4470d
Author: John Sanda <jsanda(a)redhat.com>
Date: Wed Nov 10 17:37:22 2010 -0500
Add deleteAgentPlugins method to PluginManagerBean
InventoryManagerBean now has two versions of markTypesForDeletion, one
that takes a var-args of resource type ids and the other that takes a
list of ResourceTypes. The UI has also been updated so that it now has a
functioning 'DELETE' button for agent plugins.
* looks like you accidentally committed the file
.../portal-war/0001-Initial-commit-for-CreateResourceHistoryCriteria.jav.patch
* why keep the function that takes List<ResourceType>? this method
requires
fetching more data across the line from the DB (the entire
ResourceType entity
instead of just having a type id), so I would prefer to see the
overload that takes
a list of ids stay and remove this one. the rest of our api attempts
to follow this
pattern too, where the PKs of the objects to be manipulated are used
instead of
the objects themselves (except in cases like
updateSomeObject(SomeObject)
where the object is absolutely needed as part of the method contract)
-----
commit df2031523d754206b7bb9f23584060c90c9e9f52
Author: John Sanda <jsanda(a)redhat.com>
Date: Wed Nov 10 17:55:20 2010 -0500
Don't delete a plugin if other plugins depend on it and aren't marked
for deletion
* we should add logic to the UI where selecting a plugin automatically
selects its
dependents so this case is automatically handled. probably best to
wait until the page
is migrated to gwt, which will make the logic much simpler to implement.
-----
commit 956e8426629dac531df783758749fe5af0a4fc31
Author: John Sanda <jsanda(a)redhat.com>
Date: Thu Nov 11 16:26:00 2010 -0500
Initial commit for PurgeResourceTypes, the scheduled job to remove
resource types
PurgeResourceTypes is the scheduled job that carries out the out of band
work to permanently remove resource types that have been deleted. In
this context, deleted means that the resource type's deleted flag has
been set. I have added a few methods to InventoryManagerBean, in part
to minimize the dependencies for PurgeResourceTypes which will in turn
simplfy testing.
The first method added to InventoryManagerBean is getDeletedTypes which
just queries for deleted resource types. The second method is
isReadyForPermanentRemoval. This method encapsulates the logic for
determing if a resource type can be safely removed from the system. The
last method is purgeDeletedResourceType which does the removal. This
method is (currently) just a thin wrapper around
ResourceMetadataManagerBean.completeRemoveResourceType. A lot of the
work in completeRemoveResourceType is done via cascading deletes which
might not be the most performant. This code needs further analysis (and
refactoring).
* the class should be called PurgeResourceTypeJob, to follow the
convention of everything
else that gets driven through the quartz scheduler on a periodic basis
* InventoryManagerBean.isReadyForPermanentRemoval(ResourceType) pulls
across the wire
all resources of a particular type, which could be a lot, only to see
if the result set
is empty or not. that could be a very expensive operate in
practice. if all you need
is a count, then restrict the criteria object to only return one
row. better yet, try
changing the criteria to return 0 results, then check on the value of
PageList.getTotalSize() which is the magnitude returned by the
corresponding count-query.
-----
commit bf9eda023797a5690a03b758e19fa2db24122a1d
Author: John Sanda <jsanda(a)redhat.com>
Date: Thu Nov 11 16:46:45 2010 -0500
Remember the plugins being deleted so they can be included in status
message
* You print an informative error message with id/name/md5 for the
deleted plugins, each
of which will be prefixed with "AgentPlugin". A user-friendly error
message would just
be a comma-delimited list of plugin names.
-----
commit 210bdf43c63f8d7290e9d065a86a8bfa7749f916
Author: John Sanda <jsanda(a)redhat.com>
Date: Fri Nov 12 15:25:25 2010 -0500
Filter out resource types that have already been deleted from the db
DeletedResourceTypeFilter previously only filter out resource types that
were marked deleted. If a resource in an inventory report was of a type
that was already removed from the database, it would get past the
filter. Since the type did not exist on the server, it would not get
committed into inventory, but the server would just ignore it when it
should be instead notifying the agent that it sent up a stale inventory
report.
When the agent is notified that plugin container needs to be restarted,
it now updates its plugins with the server after shutting down the PC
and prior to starting it back up. This takes care of removing plugin
that were deleted on the server.
* the additional implementation which performs a database roundtrip for
every single
resource in the report (if deletedTypes resulted in a cache miss) is
too expensive.
keep in mind, that there could be dozens and dozens of inventory
reports coming in
to each server in a large system. since we know that the
non-existence of a type implies
that the plugin was deleted, we can iterate over the InventoryReport
and collect up the
names of all plugins for all types for all resources in the report
into a List<String>.
then we can query the rhq_plugin table once to see if this collection
of plugins referenced
in the report ALL exist in the database. if any of them do not, then
we know the report
contains at least one type that no longer exists in the database.
this reduces the perf
overhead from N queries where N is the number of non-distinct
resource types in the
InventoryReport that do NOT have their deleted bits set, to 1 per
InventoryReport. the
overall perf hit here is 2 new queries for each InventoryReport
received: one to load the
types whose bits are marked deleted, and one to checked for delete
types (by checking for
deleted plugins)
-----
commit d82f9f80c788f38da7b3b791c856ba02d3821c18
Author: John Sanda <jsanda(a)redhat.com>
Date: Mon Nov 15 14:14:17 2010 -0500
Log the total execution time for purging resource types
Removing the version of InventoryManagerBean.markTypesDeleted that took
the resource type ids since it was only used in test code. The test code
has been refactored to use the version that take a list of ResourceType
objects.
Adding some javadocs to InventoryManagerBeanLocal.
* Would have preferred to keep the method that took the integer
arguments. it follows
the pattern setup in most of the rest of our APIs when operating on
things, which allows
users that don't have the entire entity object loaded from the db to
perform the action.
-----
commit 6f381c25e4fb9eae1195469e13d85f0163285ae4
Author: John Sanda <jsanda(a)redhat.com>
Date: Mon Nov 15 14:58:59 2010 -0500
Remove the actual plugin jar file from the downloads dir
When a plugin is deleted, the plugin jar that is stored in the
rhq-downloads directory needs to be deleted; otherwise, after a server
restart, the server will try to re-install the plugin.
* what if the plugin jar file is locked (which often happens on windows
boxes), it will
cause the entire plugin deletion code to bomb. is there anything we
can do if the
delete fails that would allow this to continue to work? what if we
renamed the file
instead of deleting it, would that fail in the same way due to
locking issues? mazz?
-----
commit bfd832765a787b1ab53bb0471219d7b30057569e
Author: John Sanda <jsanda(a)redhat.com>
Date: Mon Nov 15 21:22:30 2010 -0500
Updating plugin deletion logic for HA deployments
Initially I was deleting the plugin from the RHQ_PLUGIN tableas well as
deleting the plugin jar file from the file system. The problem with
deleting the plugin from the database in an HA deployment, as mazz
explained to me, is that servers other than the one that serviced the
request, will not know that the plugin has been deleted. If the agent
plugin scanner comes along and sees the plugin jar file on disk, it
would attempt to re-install the plugin.
AS far as the Plugin object/file goes, deletion of agent plugins now has
the same semantics as it does with server plugins in so far as it state
that the plugin assumes. When the agent plugin scanner runs, it now
looks at any plugins that have been deleted and removes them from the
file system.
A separate purge operation will be added to PluginManagerBean in a
subsequent commit.
* how does this compare / contrast to the deletion routine from the
commit below, which
removes the filesystem object at the time the user triggers the
removal of the type in the
UI? it seems like this code is a superset of that other code. by
the way, what triggers the
AgentPluginScanner to perform a new scan after the user initiates the
type removal by
using the UI of only one of the rhq servers?
-----
commit 81977d01b6f6f39b684e4809bf8edf3d3e4d21eb
Author: John Sanda <jsanda(a)redhat.com>
Date: Wed Nov 17 20:49:54 2010 -0500
Adding support for purging an agent plugin
I have updated the UI for agent plugins to behave similar to how it
behaves for server plugins. There is now a 'SHOW DELETED' button on the
agent plugins tab. When you click it, deleted plugins will be displayed
as well as a 'PURGE' button. Clicking the purge button will result in
the plugin being purged from the database.
The purge operation as a safeguard calls the delete operation on each
plugin before purging them.
* Not a fan of exposing deleted objects to the user. from a user's
perspective, all he or
she needs to know is that at the time they clicked the deleted
button, the plugin went
bye-bye. using deleted bits is an implementation-level detail of how
we're performing
the bulk of the actual work in an async fashion. exposing this
detail adds an extra
level of complexity for the user that the UI can probably do without.
-----
commit 51a03e5bf9ae7828dd79e0f6484705ab0729e1e5
Author: John Sanda <jsanda(a)redhat.com>
Date: Thu Nov 18 12:08:32 2010 -0500
Adding logging for meta data manager EJBs
* very, very good commit! we really should do more of this. this will
certainly make
maintaining this solution easier because if there is ever a failure
at a customer site,
we'll tell them to turn on debug logging and know precisely what went
wrong.
-----
-----
Other thoughts
* many code files - particularly the tests and builder classes - are
formatted in a very
specific way. as soon as anyone else edits and saves one of those
files, the formatting
will be lost. also want to reiterate the '*' imports used in several
files. in general,
please use standard formatting.
* tests are written in groovy, but didn't have to be. what happens if
you leave the team?
then others need to support these tests, and fix them when they will
inevitably break.
if they are not familiar with the language, then this slows us down
as a team because it
increases our maint burden. as much as i appreciate people learning
and reading about
technology on the side, we need to constrain what makes it into the
product, especially
as it pertains to supportability. unless we're going to make a
team-wide decision that
we should all learn groovy so we can write tests - this needs to be
converted to java.
in the future, things like this should always be brought to mine and
charles' attention
for consideration BEFORE attempting them, since this falls way
outside the realm of
"an implementation detail". i'm sure there will be disagree about
this, and if that's
the case let's please get on a call to discuss instead of having to
write 20 emails
back and forth. in the end, i believe this is a decision that
charles has to make, but
i'm vehemently opposed to letting new langs into any part of the code
that others will
have to support going forward. an unsupported agent or server-side
plugin that enables
other language bindings is fine, because we don't support those in
JON, and if they start
to break because they aren't being maintained we can simply take them
out of the build.
but the tests that ensure the core of the product is working as it's
supposed to does not
fall into the same category, and should be written in java.
13 years, 5 months
Re: gwt exception handling
by Ian Springer
Adding the throws clauses is the quick fix, but it doesn't address the
problem of the coregui code having to do String parsing to figure out
the root cause, since the root cause has been converted to a String and
stuck into the message of the wrapping RuntimeException:
throw new RuntimeException(ThrowableUtils.getAllExceptions(e));
For example, here's what I have to resort to in
UsersDataSource.executeAdd():
String message = caught.getMessage();
if (message != null &&
message.contains("javax.persistence.EntityExistsException")) {
Map<String, String> errorMessages = new
HashMap<String, String>();
errorMessages.put(Field.NAME, "A user named [" +
newSubject.getName() + "] already exists.");
sendValidationErrorResponse(request, response,
errorMessages);
} else {
throw new RuntimeException(caught);
}
Notice the message.contains(...) check.
It would be much preferred for either the SLSB method of the async impl
class to throw a strongly typed exception, e.g.:
throw new org.rhq.core.domain.exception.EntityExistsException("A
subject named " + subjectName + " already exists.");
then the coregui code would not have to resort to parsing the exception
message - it could instead use an instanceof check, e.g.:
if (caught instanceof
org.rhq.core.domain.exception.EntityExistsException) {
Map<String, String> errorMessages = new
HashMap<String, String>();
errorMessages.put(Field.NAME, "A user named [" +
newSubject.getName() + "] already exists.");
sendValidationErrorResponse(request, response,
errorMessages);
} else {
throw new RuntimeException(caught);
}
cleaner and more stable maintenance-wise.
Btw, I'd use "throws RuntimeException", rather than "throws Exception",
in the async impl class methods, since it's more specific.
On 12/06/2010 09:27 AM, John Mazzitelli wrote:
> You need a slight clarification:
>
>> If I understand Mazz correctly, as long as we declare that the method
>> throws Exception - even if we only ever throw RuntimeException - things
>> will "just work".
>
> If you mean to say explicitly "java.lang.RuntimeException", then yes,
> this is correct (e.g. "throw new java.lang.RuntimeException()").
>
> However, it can't be any ol' exception derived from RuntimeException.
> Even if its an exception that extends RuntimeException, if that
> exception class isn't available to the GWT client, it still won't work.
>
> So, IllegalArgumentException is fine - because its available to the
> GWT client, but "InvalidAlertDefinitionException" (which is one of
> ours), is not fine because even though its a RuntimeException, it is
> not available to the GWT client since its only in server/jar. Same
> goes for EJBException - its a RuntimeException but its not available
> to the GWT client since it knows nothing about javax.ejb.*.
--
Ian Springer
Principal Software Developer
JBoss Operations Network
Red Hat
ian.springer(a)redhat.com
13 years, 5 months
release-3.0.0 branch failure, master ok
by John Mazzitelli
The release-3.0.0 branch is failing, master is ok. After doing some
debugging, I find this is the cause... this is a git commit in master
that fixed the problem and probably has to get cherry picked over, the
git commit message is exactly the error the test is getting in
release-3.0.0 (and it is where i found the problem in my debugging):
commit 31a27cb1807ea2e0ae258d996ce968ec0f730135
Author: Ian Springer <ian.springer(a)redhat.com> 2010-09-21 11:10:38
Committer: Ian Springer <ian.springer(a)redhat.com> 2010-09-21 11:16:42
comment out some code that was causing the following unit test failure:
Tests run: 3, Failures: 1, Errors: 0, Skipped: 2, Time elapsed: 863.162
sec <<< FAILURE!
beforeClass(org.rhq.core.pc.inventory.ResourceContainerTest) Time
elapsed: 0 sec <<< FAILURE!
java.lang.RuntimeException: Cannot initialize the plugin container
at org.rhq.core.pc.plugin.PluginManager.initialize(PluginManager.java:173)
at
org.rhq.core.pc.PluginContainer.startContainerService(PluginContainer.java:391)
at org.rhq.core.pc.PluginContainer.initialize(PluginContainer.java:252)
at
org.rhq.core.pc.inventory.ResourceContainerTest.beforeClass(ResourceContainerTest.java:48)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
at
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
at java.lang.reflect.Method.invoke(Method.java:597)
at org.testng.internal.MethodHelper.invokeMethod(MethodHelper.java:607)
at org.testng.internal.Invoker.invokeConfigurationMethod(Invoker.java:417)
at org.testng.internal.Invoker.invokeConfigurations(Invoker.java:154)
at org.testng.internal.Invoker.invokeConfigurations(Invoker.java:88)
at
org.testng.internal.TestMethodWorker.invokeBeforeClassMethods(TestMethodWorker.java:167)
at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:104)
at org.testng.TestRunner.runWorkers(TestRunner.java:759)
at org.testng.TestRunner.privateRun(TestRunner.java:592)
at org.testng.TestRunner.run(TestRunner.java:486)
at org.testng.SuiteRunner.runTest(SuiteRunner.java:332)
at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:327)
at org.testng.SuiteRunner.privateRun(SuiteRunner.java:299)
at org.testng.SuiteRunner.run(SuiteRunner.java:204)
at org.testng.TestNG.createAndRunSuiteRunners(TestNG.java:877)
at org.testng.TestNG.runSuitesLocally(TestNG.java:842)
at org.testng.TestNG.run(TestNG.java:751)
at
org.apache.maven.surefire.testng.TestNGExecutor.run(TestNGExecutor.java:62)
at
org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.execute(TestNGDirectoryTestSuite.java:141)
at org.apache.maven.surefire.Surefire.run(Surefire.java:180)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
at
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
at java.lang.reflect.Method.invoke(Method.java:597)
at
org.apache.maven.surefire.booter.SurefireBooter.runSuitesInProcess(SurefireBooter.java:350)
at
org.apache.maven.surefire.booter.SurefireBooter.main(SurefireBooter.java:1021)
Caused by: org.rhq.core.clientapi.agent.PluginContainerException: A
valid plugin JAR URL must be supplied.
at
org.rhq.core.clientapi.descriptor.AgentPluginDescriptorUtil.loadPluginDescriptorFromUrl(AgentPluginDescriptorUtil.java:296)
at
org.rhq.core.pc.plugin.PluginDescriptorLoader.loadPluginDescriptor(PluginDescriptorLoader.java:67)
at org.rhq.core.pc.plugin.PluginManager.loadPlugin(PluginManager.java:316)
at org.rhq.core.pc.plugin.PluginManager.initialize(PluginManager.java:168)
... 32 more
Results :
Failed tests:
beforeClass(org.rhq.core.pc.inventory.ResourceContainerTest)
modules/core/plugin-container/src/main/java/org/rhq/core/pc/plugin/PluginManager.java
index 2c64ff1..d9976cb 100644
---
a/modules/core/plugin-container/src/main/java/org/rhq/core/pc/plugin/PluginManager.java
+++
b/modules/core/plugin-container/src/main/java/org/rhq/core/pc/plugin/PluginManager.java
@@ -163,9 +163,11 @@
}
} else {
+ // TODO (ips, 10/21/10): I don't think this block of
code is needed any longer. I am commenting it for
+ // now, but it should eventually
be deleted.
// Loading a null plugin loads the plugin using the
current classloader (for unit test testing)
- log.info("Loading the null plugin which uses
non-isolated classloader");
- loadPlugin(null, thisClassLoader);
+ //log.info("Loading the null plugin which uses
non-isolated classloader");
+ //loadPlugin(null, thisClassLoader);
}
} catch (Exception e) {
shutdown(); // have to clean up the environments (e.g.
unpacked jars) we might have already created
@@ -379,7 +381,6 @@
*
* @param pluginName the name of the plugin to obtain
dependency URLs for
* @param pluginNamesUrls map of all known plugin names and their
plugin jar URLs
- * @param graph the dependency graph
* @param allUrls where the results will be stored
*
* TODO: Use it or lose it - this used to be needed, keeping this
around just in case it needs to be resurrected
13 years, 5 months