Bug 991149 - Refactor Plugin Container Services initialization
Summary: Refactor Plugin Container Services initialization
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: RHQ Project
Classification: Other
Component: Agent
Version: 4.8
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
: RHQ 4.10
Assignee: Thomas Segismont
QA Contact: Mike Foley
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2013-08-01 17:58 UTC by Elias Ross
Modified: 2014-05-16 09:16 UTC (History)
5 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2014-05-16 09:16:59 UTC
Embargoed:


Attachments (Terms of Use)
Patch for master (69656f42348a) (203.42 KB, application/mbox)
2013-08-01 17:58 UTC, Elias Ross
no flags Details
patch rebased on master (69656f4234) (203.56 KB, patch)
2013-08-14 19:29 UTC, Elias Ross
no flags Details | Diff


Links
System ID Private Priority Status Summary Last Updated
Red Hat Bugzilla 949065 0 unspecified CLOSED NPE when events published before plugin container fully starts 2021-02-22 00:41:40 UTC

Internal Links: 949065

Description Elias Ross 2013-08-01 17:58:01 UTC
Created attachment 781694 [details]
Patch for master (69656f42348a)

See Bug 949065

From the patch notes:
'
    Since there have been other race conditions in the past (as documented in the
    code in a few spots), it feels there is an overall design issue at fault:
    Objects are initialized in arbitrary order, enforced by the use of 'listeners',
    but this was done as a reaction to what bugs were found.
    
    The solution employed here is to use constructors and immutability when
    possible. Handles to other components should be done using object fields, not
    an object registry/singleton pattern. I tried to eliminate the use of
    PluginContainer.getInstance().getXXX(), since in many cases references could be
    null depending on the order of how objects are initialized. It also makes clear
    what objects really depend on what objects. The use of 'PC.getInstance()'
    should probably be entirely removed, since it's bug-prone, but I'll leave the
    rest of the cleanup for later.
    
    Testability is generally improved, as test/mock instances can simply be
    'injected' using the constructor, not by having to create methods
    that get overridden.

'

Comment 1 Elias Ross 2013-08-14 19:29:00 UTC
Created attachment 786683 [details]
patch rebased on master (69656f4234)

Comment 2 Thomas Segismont 2013-11-21 14:28:02 UTC
Changed the title to make it clear it's not about agent plugin component API - in case it was unclear for anybody other than me :)

Comment 3 Thomas Segismont 2013-11-21 14:43:01 UTC
Finished to apply the patch on master (many conflicts and changes in some test classes outside the plugin container module needed).

The PC module test suite passes

Currently smoke testing. Looks good so far.

Remaining:
* probe job passing
* api-check job passing
* in depth review

Comment 4 Thomas Segismont 2013-11-21 15:18:06 UTC
(In reply to Thomas Segismont from comment #3)
> Finished to apply the patch on master 

It's currently in a bug branch (bug/991149)
https://git.fedorahosted.org/cgit/rhq/rhq.git/log/?h=bug/949065

Comment 5 Elias Ross 2013-11-21 18:42:00 UTC
Sorry about the patch complication. I tried my best to keep the changes in sync, at least at the moment when I submitted my patch. It may have worked better to apply the changes to the commit I patched on top of, then rebased the work.

Actually looks like there's a tool for that: https://github.com/niklasf/git-rebase-patch

Still I realize my changes were fairly invasive so it might not have saved any significant work. :-(

Feel free to send a patch back that needs attention.

Comment 6 Thomas Segismont 2014-02-05 10:51:42 UTC
Merged in master

commit 79a09a0a5a5402539f7a54668813f60ba274ce5a
Author: Elias Ross <genman>
Date:   Thu Nov 21 15:45:20 2013 +0100

Comment 7 Heiko W. Rupp 2014-04-23 12:29:49 UTC
Bulk closing of 4.10 issues.

If an issue is not solved for you, please open a new BZ (or clone the existing one) with a version designator of 4.10.

Comment 8 John Mazzitelli 2014-05-12 13:44:02 UTC
Reopening this - this introduced some very bad regressions. See bug #1096332

Comment 9 Lukas Krejci 2014-05-16 09:16:59 UTC
Seems to have been a false alarm. Closing again, the regressions weren't confirmed.


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