Uploaded image for project: 'OFBiz'
  1. OFBiz
  2. OFBIZ-8337

Refactor and simplify the startup sequence in OFBiz

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: Upcoming Release
    • Fix Version/s: Upcoming Release
    • Component/s: base, start
    • Labels:
      None

      Description

      The startup sequence in OFBiz is highly complex and requires improvements on multiple levels including:

      • The entire classpath buildup logic and org.apache.ofbiz.base.start.Classpath needs to be removed. The original idea of classpath isolation between the components created many more problems than it solved, and right now the classpath construction is the responsibility of the build system.
      • The custom classloader needs to be removed as well together with the classpath mentioned above.
      • The StartupLoader interface should remove the start() method and just have two methods, load and unload.
      • The startup sequence should have only one StartupLoader, not an array of startup loaders. This StartupLoader (implemented as ContainerLoader) is the only class responsible for bootstrapping OFBiz
      • The ContainerLoader needs to be completely refactored, not only to remove the start() method but also to cleanup the very messy logic currently residing there.
      1. OFBIZ-8337.patch
        86 kB
        Taher Alkhateeb
      2. OFBIZ-8337-2.patch
        29 kB
        Taher Alkhateeb
      3. OFBIZ-8337-3.patch
        41 kB
        Taher Alkhateeb
      4. OFBIZ-8337-3.patch
        39 kB
        Taher Alkhateeb

        Issue Links

          Activity

          Hide
          toashishvijay Ashish Vijaywargiya added a comment -

          Very good task. Thanks Taher for working on this!

          Show
          toashishvijay Ashish Vijaywargiya added a comment - Very good task. Thanks Taher for working on this!
          Hide
          taher Taher Alkhateeb added a comment -

          Thank you, very kind of you Ashish I will submit a patch (I'm still in the middle of the pain) and I would really appreciate help and feedback if you have time.

          Show
          taher Taher Alkhateeb added a comment - Thank you, very kind of you Ashish I will submit a patch (I'm still in the middle of the pain) and I would really appreciate help and feedback if you have time.
          Hide
          taher Taher Alkhateeb added a comment - - edited

          I believe this is an abandoned feature to remove from the framework with this refactoring exercise to reduce the complexity of the startup sequence. Thank you Pierre for the link

          Show
          taher Taher Alkhateeb added a comment - - edited I believe this is an abandoned feature to remove from the framework with this refactoring exercise to reduce the complexity of the startup sequence. Thank you Pierre for the link
          Show
          jacques.le.roux Jacques Le Roux added a comment - Was http://svn.apache.org/viewvc?rev=1076529&view=rev on HipChat
          Hide
          taher Taher Alkhateeb added a comment -

          This is a major big patch that applies the following:

          • Rename ContainerConfig.Container to ContainerConfig.Configuration, the old
            nameing was very confusing as it confuses it with actual containers
          • Change the signature of ContainerLoader and Container to accept
            List<StartupCommand> instead of String[]. And also migrate the adapter for
            converting these args into a standalone class called
            StartupCommandsToArgsAdapter
          • Remove the LockedBy annotation as it is not used
          • Substantially simplify the ContainerLoader load and unload logic and delete
            the start logic (merge it with load). The interface StartupLoader was updated
            accordingly
          • Remove old logic to load containers in hot-deploy components
          • Remove printThreadDump output from ContainerLoader which used to exist for
            debugging purposes and is no longer necessary
          • Cleanup the AdminServer startup logic
          • Change the StartupControlPanel logic to use only one StartupLoader instead
            of a list. The only implementation is ContainerLoader
          • Remove the synchronized blocks in StartupControlPanel due to having only one
            loader
          • Update the startup .properties files to reflect the changes in the startup
            logic
          • Update the TestRunContainer to use List<StartupCommand> in its logic
          Show
          taher Taher Alkhateeb added a comment - This is a major big patch that applies the following: Rename ContainerConfig.Container to ContainerConfig.Configuration, the old nameing was very confusing as it confuses it with actual containers Change the signature of ContainerLoader and Container to accept List<StartupCommand> instead of String[]. And also migrate the adapter for converting these args into a standalone class called StartupCommandsToArgsAdapter Remove the LockedBy annotation as it is not used Substantially simplify the ContainerLoader load and unload logic and delete the start logic (merge it with load). The interface StartupLoader was updated accordingly Remove old logic to load containers in hot-deploy components Remove printThreadDump output from ContainerLoader which used to exist for debugging purposes and is no longer necessary Cleanup the AdminServer startup logic Change the StartupControlPanel logic to use only one StartupLoader instead of a list. The only implementation is ContainerLoader Remove the synchronized blocks in StartupControlPanel due to having only one loader Update the startup .properties files to reflect the changes in the startup logic Update the TestRunContainer to use List<StartupCommand> in its logic
          Hide
          taher Taher Alkhateeb added a comment -

          This is a note to myself. To complete refactoring the startup sequence I need to also refactor all the containers themselves, and this would require substantial changes to the API signatures. Perhaps we can even get rid of the idea of a container all together for more efficiency. The containers are listed below to start working on them:

          • framework/entity/src/main/java/org/apache/ofbiz/entity/DelegatorContainer.java
          • framework/base/src/main/java/org/apache/ofbiz/base/container/ComponentContainer.java
          • framework/base/src/main/java/org/apache/ofbiz/base/container/JustLoadComponentsContainer.java
          • framework/base/src/main/java/org/apache/ofbiz/base/container/NamingServiceContainer.java
          • framework/service/src/main/java/org/apache/ofbiz/service/rmi/RmiServiceContainer.java
          • framework/service/src/main/java/org/apache/ofbiz/service/mail/JavaMailContainer.java
          • framework/service/src/main/java/org/apache/ofbiz/service/ServiceContainer.java
          • framework/entityext/src/main/java/org/apache/ofbiz/entityext/data/EntityDataLoadContainer.java
          • framework/testtools/src/main/java/org/apache/ofbiz/testtools/TestRunContainer.java
          • framework/catalina/src/main/java/org/apache/ofbiz/catalina/container/CatalinaContainer.java
          • specialpurpose/birt/src/main/java/org/apache/ofbiz/birt/container/BirtContainer.java
          Show
          taher Taher Alkhateeb added a comment - This is a note to myself. To complete refactoring the startup sequence I need to also refactor all the containers themselves, and this would require substantial changes to the API signatures. Perhaps we can even get rid of the idea of a container all together for more efficiency. The containers are listed below to start working on them: framework/entity/src/main/java/org/apache/ofbiz/entity/DelegatorContainer.java framework/base/src/main/java/org/apache/ofbiz/base/container/ComponentContainer.java framework/base/src/main/java/org/apache/ofbiz/base/container/JustLoadComponentsContainer.java framework/base/src/main/java/org/apache/ofbiz/base/container/NamingServiceContainer.java framework/service/src/main/java/org/apache/ofbiz/service/rmi/RmiServiceContainer.java framework/service/src/main/java/org/apache/ofbiz/service/mail/JavaMailContainer.java framework/service/src/main/java/org/apache/ofbiz/service/ServiceContainer.java framework/entityext/src/main/java/org/apache/ofbiz/entityext/data/EntityDataLoadContainer.java framework/testtools/src/main/java/org/apache/ofbiz/testtools/TestRunContainer.java framework/catalina/src/main/java/org/apache/ofbiz/catalina/container/CatalinaContainer.java specialpurpose/birt/src/main/java/org/apache/ofbiz/birt/container/BirtContainer.java
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Hi Taher,

          Thanks for the details. I reviewed, I like it, nice code. I agree about removing old/unused stuff.

          I tested, all is OK with me, notably ofbizBackground and terminateOfbiz.

          Show
          jacques.le.roux Jacques Le Roux added a comment - Hi Taher, Thanks for the details. I reviewed, I like it, nice code. I agree about removing old/unused stuff. I tested, all is OK with me, notably ofbizBackground and terminateOfbiz.
          Hide
          taher Taher Alkhateeb added a comment -

          Thank you Jacques, my first patch (modified with some further renaming of container to configuration) is committed in r1765127

          Show
          taher Taher Alkhateeb added a comment - Thank you Jacques, my first patch (modified with some further renaming of container to configuration) is committed in r1765127
          Hide
          taher Taher Alkhateeb added a comment -

          As discussed in http://markmail.org/message/vz4ggmkgqbsfxnut I realized that I introduced a regression from r1765127 which I reverted partially in r1767866. Now the issue is resolved and I will continue cleaning up the code base while paying very close attention to the custom classloader

          Show
          taher Taher Alkhateeb added a comment - As discussed in http://markmail.org/message/vz4ggmkgqbsfxnut I realized that I introduced a regression from r1765127 which I reverted partially in r1767866. Now the issue is resolved and I will continue cleaning up the code base while paying very close attention to the custom classloader
          Hide
          taher Taher Alkhateeb added a comment -

          This is a new major patch for refactoring the startup sequence with the following highlights:

          • simplify and cleanup the ofbiz-containers.xml to have only one entry
          • substantially simplify the ComponentLoaderConfig class and remove unnecessary defined state
          • delete the JustLoadComponentsContainer (used to exist for the server command --testlist which no longer exists)
          • Fully refactor the ComponentContainer class and breakup the messy logic into chunks of private methods. The methods are also properly documented.

          It is still not up to notch, but compared to the absolute mess it used to be, the code is now much more readable.

          Show
          taher Taher Alkhateeb added a comment - This is a new major patch for refactoring the startup sequence with the following highlights: simplify and cleanup the ofbiz-containers.xml to have only one entry substantially simplify the ComponentLoaderConfig class and remove unnecessary defined state delete the JustLoadComponentsContainer (used to exist for the server command --testlist which no longer exists) Fully refactor the ComponentContainer class and breakup the messy logic into chunks of private methods. The methods are also properly documented. It is still not up to notch, but compared to the absolute mess it used to be, the code is now much more readable.
          Hide
          taher Taher Alkhateeb added a comment - - edited

          Note to self on container startup and defined loaders

          Container name Defined loaders Notes
          ComponentContainer main,rmi,load-data,test Starts before all other containers
          DelegatorContainer main  
          ServiceContainer main,rmi,load-data,test  
          CatalinaContainer main  
          BirtContainer main  
          NamingServiceContainer rmi  
          RmiServiceContainer rmi  
          EntityDataLoadContainer load-data  
          TestRunContainer test  
          JavaMailContainer all (none selected) commented out by default
          Show
          taher Taher Alkhateeb added a comment - - edited Note to self on container startup and defined loaders Container name Defined loaders Notes ComponentContainer main,rmi,load-data,test Starts before all other containers DelegatorContainer main   ServiceContainer main,rmi,load-data,test   CatalinaContainer main   BirtContainer main   NamingServiceContainer rmi   RmiServiceContainer rmi   EntityDataLoadContainer load-data   TestRunContainer test   JavaMailContainer all (none selected) commented out by default
          Hide
          jacopoc Jacopo Cappellato added a comment -

          Taher Alkhateeb thanks for your work.
          I have reviewed your patch which looks great and also applied it to my local environment: smoke testing was successful.
          I suggest to commit it to trunk to perform some more tests and plan for the next incremental steps.
          One of these could be that of getting rid completely of the custom NativeLibClassloader and remove the code that calls its addNativeClassPath(...) method: it will be one step in the direction of having a simpler to deploy system.

          Show
          jacopoc Jacopo Cappellato added a comment - Taher Alkhateeb thanks for your work. I have reviewed your patch which looks great and also applied it to my local environment: smoke testing was successful. I suggest to commit it to trunk to perform some more tests and plan for the next incremental steps. One of these could be that of getting rid completely of the custom NativeLibClassloader and remove the code that calls its addNativeClassPath(...) method: it will be one step in the direction of having a simpler to deploy system.
          Hide
          taher Taher Alkhateeb added a comment -

          Hi Jacopo,

          Sure, I will do some further cleanup and commit. However, now that I have investigated the code thoroughly, I think it's not very simple to remove the NativeLibClassLoader. The reason is that the addURL method is exposed as public whereas its parent class URLClassLoader defines it as protected. The addURL method is necessary for the current functioning of the project (the <classpath...> entries in components rely on it to add important files to the classpath.

          Gradle takes care of external libraries, but I think it should not also specify what each component wants.

          I've been thinking about this for a while, one solution that comes to my mind is maybe to defer the initialization of the class loader and instead build the classpath object, and only after the classpath object is fully materialized do we initiate the URLClassLoader with the classpath passed into the constructor. I need to think about this for a while though.

          Show
          taher Taher Alkhateeb added a comment - Hi Jacopo, Sure, I will do some further cleanup and commit. However, now that I have investigated the code thoroughly, I think it's not very simple to remove the NativeLibClassLoader. The reason is that the addURL method is exposed as public whereas its parent class URLClassLoader defines it as protected. The addURL method is necessary for the current functioning of the project (the <classpath...> entries in components rely on it to add important files to the classpath. Gradle takes care of external libraries, but I think it should not also specify what each component wants. I've been thinking about this for a while, one solution that comes to my mind is maybe to defer the initialization of the class loader and instead build the classpath object, and only after the classpath object is fully materialized do we initiate the URLClassLoader with the classpath passed into the constructor. I need to think about this for a while though.
          Hide
          soledad Nicolas Malin added a comment -

          Hi Taher,
          It's also good refactoring for me.

          I just found a typo at the line 432 on the last patch but it's a nitpicking

          Show
          soledad Nicolas Malin added a comment - Hi Taher, It's also good refactoring for me. I just found a typo at the line 432 on the last patch but it's a nitpicking
          Hide
          taher Taher Alkhateeb added a comment -

          Hi Nicolas

          I hope I'm not going blind but Line 432 has:

          +        if(config == null) {
          

          I'm not sure if I'm looking in the right place?

          Show
          taher Taher Alkhateeb added a comment - Hi Nicolas I hope I'm not going blind but Line 432 has: + if (config == null ) { I'm not sure if I'm looking in the right place?
          Hide
          soledad Nicolas Malin added a comment -

          [wishpering] missing a space between if and ( [wishpering]

          You push this on trunk ?

          Show
          soledad Nicolas Malin added a comment - [wishpering] missing a space between if and ( [wishpering] You push this on trunk ?
          Hide
          taher Taher Alkhateeb added a comment -

          Ahhh okay coding style, I always forget that space.

          Yeah, I'm experimenting with removing the NativeLibClassLoader right now but also cleaning up some of the code before the final commit.

          Show
          taher Taher Alkhateeb added a comment - Ahhh okay coding style, I always forget that space. Yeah, I'm experimenting with removing the NativeLibClassLoader right now but also cleaning up some of the code before the final commit.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          I began to review last week but got sidetracked, I hope to have a new look, could be after commit...

          Show
          jacques.le.roux Jacques Le Roux added a comment - I began to review last week but got sidetracked, I hope to have a new look, could be after commit...
          Hide
          jacopoc Jacopo Cappellato added a comment -

          Thanks for the insight Taher!
          I will have to look again at the classloader code we have since it is a long time since I did.
          I would recommend that you commit this work and we postpone the further (if any) work to remove NativeLibClassLoader after your commit.

          Show
          jacopoc Jacopo Cappellato added a comment - Thanks for the insight Taher! I will have to look again at the classloader code we have since it is a long time since I did. I would recommend that you commit this work and we postpone the further (if any) work to remove NativeLibClassLoader after your commit.
          Hide
          taher Taher Alkhateeb added a comment -

          Work committed in r1772256. Next I'll focus my attention on the custom class loader

          Show
          taher Taher Alkhateeb added a comment - Work committed in r1772256. Next I'll focus my attention on the custom class loader
          Hide
          taher Taher Alkhateeb added a comment -

          This is another major patch which further refactors the startup sequence:

          • Delete the NativeLibClassLoader
          • Refactor the ComponentContainer and StartupControlPanel to operate without
            the NativeLibClassLoader. This substantially simplifies the code
          • Declare a URLClassLoader in ComponentContainer that is instantiated upon
            building the classpath for all components. This makes the classloader start
            in one shot
          • Simplify the Config file and remove fields that are not used. Also
            refactor some messy logic for loading the props file and other code
            improvements
          • Refactor all the .properties files for startup to have a consistent
            structure that clearly documents all available properties and the
            default value for each property. This change is also related to the
            changes applied on Config.java
          • Remove the declaration of the StartupLoader implementation class
            from all startup .properties files because only one implementation exists
            and it should be the default always.
          • Refactor the loaders retrieval code (main, rmi, test, load-data) defined
            in the startup .properties files
          • Refactor some switch statements to comply with java coding standards
          • Add the DTDs defined in base through Gradle because we removed the
            NativeLibClassLoader and the classpath buildup logic in StartupControlPanel
          Show
          taher Taher Alkhateeb added a comment - This is another major patch which further refactors the startup sequence: Delete the NativeLibClassLoader Refactor the ComponentContainer and StartupControlPanel to operate without the NativeLibClassLoader. This substantially simplifies the code Declare a URLClassLoader in ComponentContainer that is instantiated upon building the classpath for all components. This makes the classloader start in one shot Simplify the Config file and remove fields that are not used. Also refactor some messy logic for loading the props file and other code improvements Refactor all the .properties files for startup to have a consistent structure that clearly documents all available properties and the default value for each property. This change is also related to the changes applied on Config.java Remove the declaration of the StartupLoader implementation class from all startup .properties files because only one implementation exists and it should be the default always. Refactor the loaders retrieval code (main, rmi, test, load-data) defined in the startup .properties files Refactor some switch statements to comply with java coding standards Add the DTDs defined in base through Gradle because we removed the NativeLibClassLoader and the classpath buildup logic in StartupControlPanel
          Hide
          taher Taher Alkhateeb added a comment -

          Re-attaching the patch with an additional modification to Classpath.java to remove all the native libraries stuff

          Show
          taher Taher Alkhateeb added a comment - Re-attaching the patch with an additional modification to Classpath.java to remove all the native libraries stuff
          Hide
          jacopoc Jacopo Cappellato added a comment -

          I have applied and tested your latest patch and I didn't spot any issues with it. Thank you.

          Show
          jacopoc Jacopo Cappellato added a comment - I have applied and tested your latest patch and I didn't spot any issues with it. Thank you.
          Hide
          taher Taher Alkhateeb added a comment -

          Committed latest patch with minor rework in r1772879

          Show
          taher Taher Alkhateeb added a comment - Committed latest patch with minor rework in r1772879
          Hide
          taher Taher Alkhateeb added a comment -

          Although other containers still require substantial refactoring, I believe the objective of refactoring the startup sequence is mostly complete now. The other containers represent code that is considered a "feature" more than a startup logic (entity engine, service engine, etc ...). Hence, refactoring those is better done in a separate task

          Show
          taher Taher Alkhateeb added a comment - Although other containers still require substantial refactoring, I believe the objective of refactoring the startup sequence is mostly complete now. The other containers represent code that is considered a "feature" more than a startup logic (entity engine, service engine, etc ...). Hence, refactoring those is better done in a separate task

            People

            • Assignee:
              taher Taher Alkhateeb
              Reporter:
              taher Taher Alkhateeb
            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development