Bug 991149 - Refactor Plugin Container Services initialization
Refactor Plugin Container Services initialization
Product: RHQ Project
Classification: Other
Component: Agent (Show other bugs)
Unspecified Unspecified
unspecified Severity unspecified (vote)
: ---
: RHQ 4.10
Assigned To: Thomas Segismont
Mike Foley
: Reopened
Depends On:
  Show dependency treegraph
Reported: 2013-08-01 13:58 EDT by Elias Ross
Modified: 2014-05-16 05:16 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2014-05-16 05:16:59 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

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

  None (edit)
Description Elias Ross 2013-08-01 13:58:01 EDT
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 15:29:00 EDT
Created attachment 786683 [details]
patch rebased on master (69656f4234)
Comment 2 Thomas Segismont 2013-11-21 09:28:02 EST
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 09:43:01 EST
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.

* probe job passing
* api-check job passing
* in depth review
Comment 4 Thomas Segismont 2013-11-21 10:18:06 EST
(In reply to Thomas Segismont from comment #3)
> Finished to apply the patch on master 

It's currently in a bug branch (bug/991149)
Comment 5 Elias Ross 2013-11-21 13:42:00 EST
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 05:51:42 EST
Merged in master

commit 79a09a0a5a5402539f7a54668813f60ba274ce5a
Author: Elias Ross <genman@noderunner.net>
Date:   Thu Nov 21 15:45:20 2013 +0100
Comment 7 Heiko W. Rupp 2014-04-23 08:29:49 EDT
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 09:44:02 EDT
Reopening this - this introduced some very bad regressions. See bug #1096332
Comment 9 Lukas Krejci 2014-05-16 05:16:59 EDT
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.