Details

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

      Description

      The implementation of CatalinaContainer.java is currently very verbose, has replicated code, and is difficult to navigate.

      The objective of this JIRA is to breakup the logic into smaller manageable functions to allow for further enhancements in the future.

      1. OFBIZ-9392.patch
        47 kB
        Taher Alkhateeb
      2. OFBIZ-9392.patch
        47 kB
        Taher Alkhateeb
      3. OFBIZ-9392.patch
        48 kB
        Taher Alkhateeb
      4. OFBIZ-9392.patch
        48 kB
        Taher Alkhateeb

        Issue Links

          Activity

          Hide
          taher Taher Alkhateeb added a comment -

          Attaching a patch for refactoring the CatalinaContainer

          No functional change, but an almost complete rewrite of the tomcat container
          with the following highlights:

          • breakup the logic in init() many smaller functions each specializing in one
            thing
          • Unify the host creation logic between main host and context-specific hosts
          • introduce streams and lambdas where appropriate
          • rename loadComponents() to loadWebapps()
          • rename createContext() to createCallableContext()
          • rename configureContext() to prepareContext()
          • remove instance variables that are not necessary / redundant for operating
            the container correctly and refactor the code logic accordingly
          • remove unnecessary valve comments and point to documentation URL
          • remove any commented out code
          • remove the static block for initializing SSLUtil.loadJsseProperties(). This
            code is already called and hence redundant
          • remove redundant / dead / unused code
          • add missing FilterDef to context
          • lots and lots of re-arranging and small code improvements
          Show
          taher Taher Alkhateeb added a comment - Attaching a patch for refactoring the CatalinaContainer No functional change, but an almost complete rewrite of the tomcat container with the following highlights: breakup the logic in init() many smaller functions each specializing in one thing Unify the host creation logic between main host and context-specific hosts introduce streams and lambdas where appropriate rename loadComponents() to loadWebapps() rename createContext() to createCallableContext() rename configureContext() to prepareContext() remove instance variables that are not necessary / redundant for operating the container correctly and refactor the code logic accordingly remove unnecessary valve comments and point to documentation URL remove any commented out code remove the static block for initializing SSLUtil.loadJsseProperties(). This code is already called and hence redundant remove redundant / dead / unused code add missing FilterDef to context lots and lots of re-arranging and small code improvements
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Hi Taher,

          Thanks for your detailled comment, I expect to review this next week...

          Show
          jacques.le.roux Jacques Le Roux added a comment - Hi Taher, Thanks for your detailled comment, I expect to review this next week...
          Hide
          taher Taher Alkhateeb added a comment -

          Updated patch that removes the code snippet below as it is commented out in the original file

          cluster.registerManager(manager);
          
          Show
          taher Taher Alkhateeb added a comment - Updated patch that removes the code snippet below as it is commented out in the original file cluster.registerManager(manager);
          Hide
          taher Taher Alkhateeb added a comment -

          Further refactoring to simplify some of the complex methods including prepareTomcatConnectors(...), isContextDistributable(...)

          Show
          taher Taher Alkhateeb added a comment - Further refactoring to simplify some of the complex methods including prepareTomcatConnectors(...), isContextDistributable(...)
          Hide
          taher Taher Alkhateeb added a comment -

          I figured out that it might be possible to unify the webapps with a little bit of work using the DirResourceSet class. I need to do some extra homework on this, but keeping this note in JIRA as a reference for the future.

          Show
          taher Taher Alkhateeb added a comment - I figured out that it might be possible to unify the webapps with a little bit of work using the DirResourceSet class. I need to do some extra homework on this, but keeping this note in JIRA as a reference for the future.
          Hide
          taher Taher Alkhateeb added a comment -

          New attachment with minor formatting and code improvements.

          Show
          taher Taher Alkhateeb added a comment - New attachment with minor formatting and code improvements.
          Hide
          jacopoc Jacopo Cappellato added a comment -

          I have reviewed the patch and it looks good to me, thank you for your work!
          I suggest to commit the work to trunk: further tests and enhancements can be done there.

          Show
          jacopoc Jacopo Cappellato added a comment - I have reviewed the patch and it looks good to me, thank you for your work! I suggest to commit the work to trunk: further tests and enhancements can be done there.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Hi Taher,

          Thanks for your patience. I finally found a continuous hour+ to review your patch. I like it, notably the use of streams and lambdas. And the the class is much easier to read now. This patch is hard to review (much lines moved) so I'd not say that I could not have missed anything but I checked many points. Good catch on FilterDef. Of course tests and running OFBiz are fine.

          I'd though still need to be used to things like

          -        for (Map.Entry<String, String> entry: initParameters.entrySet()) {
          -            context.addParameter(entry.getKey(), entry.getValue());
          -        }
          +        initParameters.entrySet().forEach(entry -> context.addParameter(entry.getKey(), entry.getValue()));
          

          But after all syntax sugar is only that and it somehow compares to with Groovy way.

          +1 for commit.

          Show
          jacques.le.roux Jacques Le Roux added a comment - Hi Taher, Thanks for your patience. I finally found a continuous hour+ to review your patch. I like it, notably the use of streams and lambdas. And the the class is much easier to read now. This patch is hard to review (much lines moved) so I'd not say that I could not have missed anything but I checked many points. Good catch on FilterDef. Of course tests and running OFBiz are fine. I'd though still need to be used to things like - for (Map.Entry< String , String > entry: initParameters.entrySet()) { - context.addParameter(entry.getKey(), entry.getValue()); - } + initParameters.entrySet().forEach(entry -> context.addParameter(entry.getKey(), entry.getValue())); But after all syntax sugar is only that and it somehow compares to with Groovy way. +1 for commit.
          Hide
          mbrohl Michael Brohl added a comment -

          Hi Taher Alkhateeb,

          wow, great work, thank you very much! Readability improved a lot!

          I have read the code, run the tests and checked OFBiz works fine. I haven't checked the clustering functionality because I didn't have the time to setup a clustering environment to really test it. From the code, everything looks fine to me.

          Thanks and regards,
          Michael

          Just one minor note: we currently set

          context.setJ2EEServer("OFBiz Container 3.1");
          

          I guess the "3.1" comes from ancient times and think we should remove it. It doesn't hurt because it seems only to be displayed in the JMX context but it looks strange.

          +1 for committing it to the codebase.

          Show
          mbrohl Michael Brohl added a comment - Hi Taher Alkhateeb , wow, great work, thank you very much! Readability improved a lot! I have read the code, run the tests and checked OFBiz works fine. I haven't checked the clustering functionality because I didn't have the time to setup a clustering environment to really test it. From the code, everything looks fine to me. Thanks and regards, Michael Just one minor note: we currently set context.setJ2EEServer( "OFBiz Container 3.1" ); I guess the "3.1" comes from ancient times and think we should remove it. It doesn't hurt because it seems only to be displayed in the JMX context but it looks strange. +1 for committing it to the codebase.
          Hide
          taher Taher Alkhateeb added a comment -

          Committed work in r1799708. Thank you everyone for your valuable help and feedback.

          Show
          taher Taher Alkhateeb added a comment - Committed work in r1799708. Thank you everyone for your valuable help and feedback.
          Hide
          mbrohl Michael Brohl added a comment -

          I think you can resolve this issue, Taher Alkhateeb

          Show
          mbrohl Michael Brohl added a comment - I think you can resolve this issue, Taher Alkhateeb
          Hide
          taher Taher Alkhateeb added a comment -

          Closing issue. Thank you all for your help.

          Show
          taher Taher Alkhateeb added a comment - Closing issue. Thank you all for your help.

            People

            • Assignee:
              taher Taher Alkhateeb
              Reporter:
              taher Taher Alkhateeb
            • Votes:
              2 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development