detailed code review for delete-agent-plugin branch

Joseph Marques jmarques at redhat.com
Thu Dec 2 16:41:28 UTC 2010


commit eded9dd4d1657e25f2f661801ecbc2c11b12214d
Author: John Sanda <jsanda at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: https://fedorahosted.org/pipermail/rhq-devel/attachments/20101202/ec91b317/attachment-0001.html 


More information about the rhq-devel mailing list