Bug 991149

Summary: Refactor Plugin Container Services initialization
Product: [Other] RHQ Project Reporter: Elias Ross <genman>
Component: AgentAssignee: Thomas Segismont <tsegismo>
Status: CLOSED CURRENTRELEASE QA Contact: Mike Foley <mfoley>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: 4.8CC: hrupp, jshaughn, lkrejci, mazz, tsegismo
Target Milestone: ---Keywords: Reopened
Target Release: RHQ 4.10   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2014-05-16 09:16:59 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
Patch for master (69656f42348a)
none
patch rebased on master (69656f4234) none

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.