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

Refactor OFBiz containers and remove StartupCommandToArgsAdapter

    Details

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

      Description

      We have already refactored the ComponentContainer and CatalinaContainer in OFBIZ-8337 and OFBIZ-9392. The objective of this JIRA is to apply the following:

      • Refactor EntityDataLoadContainer to simplify the code and remove dependencies on "args" and convert instead the logic to use StartupCommands
      • Refactor TestRunContainer the same way
      • Delete the class StartupCommandToArgsAdapter
      • Update README.md to reflect the changes and updates and cleanup old references.
      1. log with errors.txt
        969 kB
        Jacques Le Roux
      2. OFBIZ-9441.patch
        87 kB
        Taher Alkhateeb
      3. OFBIZ-9441.patch
        88 kB
        Taher Alkhateeb
      4. OFBIZ-9441.patch
        86 kB
        Taher Alkhateeb
      5. OFBIZ-9441-fix-continue-on-failure.patch
        3 kB
        Taher Alkhateeb

        Issue Links

          Activity

          Hide
          taher Taher Alkhateeb added a comment -

          Work is complete and thankfully we didn't get any complaints or errors so far. Closing

          Show
          taher Taher Alkhateeb added a comment - Work is complete and thankfully we didn't get any complaints or errors so far. Closing
          Hide
          taher Taher Alkhateeb added a comment -

          I updated the code to remove any conflicts and committed in r1805947. Thank you for your help in review and testing

          Show
          taher Taher Alkhateeb added a comment - I updated the code to remove any conflicts and committed in r1805947. Thank you for your help in review and testing
          Hide
          jacques.le.roux Jacques Le Roux added a comment - - edited

          BTW for the point 4, about the syntax

           -l,--load-data <key=value>   Creates tables/load data e.g:
                                        -l readers=seed,demo,ext
                                        -l file=/tmp/dataload1.xml,/tmp/dataload2.xml
                                        -l dir=directory/of/files
                                        -l component=base
                                        -l delegator=default
                                        -l group=org.apache.ofbiz
                                        -l timeout=7200
                                        -l create-pks
                                        -l drop-pks
                                        -l create-constraints
                                        -l drop-constraints
                                        -l create-fks
                                        -l maintain-txs
                                        -l try-inserts
                                        -l repair-columns
                                        -l continue-on-failure
          

          And the use of several -l or--load-data, it's explained in the note indeed. Sorry for the confusion.

          Show
          jacques.le.roux Jacques Le Roux added a comment - - edited BTW for the point 4, about the syntax -l,--load-data <key=value> Creates tables/load data e.g: -l readers=seed,demo,ext -l file=/tmp/dataload1.xml,/tmp/dataload2.xml -l dir=directory/of/files -l component=base -l delegator= default -l group=org.apache.ofbiz -l timeout=7200 -l create-pks -l drop-pks -l create-constraints -l drop-constraints -l create-fks -l maintain-txs -l try -inserts -l repair-columns -l continue -on-failure And the use of several -l or--load-data, it's explained in the note indeed. Sorry for the confusion.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Just an idea (did not check) could it not be a transaction issue?

          Show
          jacques.le.roux Jacques Le Roux added a comment - Just an idea (did not check) could it not be a transaction issue?
          Hide
          jacques.le.roux Jacques Le Roux added a comment - - edited

          3. Right
          4 Yep I thought so, just tried again with

          g cleanAll eclipse build "ofbiz -l readers=seed,demo,ext -l delegator=default -l group=org.apache.ofbiz"

          same errors than with

          java -jar build/libs/ofbiz.jar -l readers=seed,demo,ext

          5. You mean that it's about the order (because loadAll works)? Of course using

          g cleanAll eclipse build "ofbiz -l readers=seed,demo,ext"

          I get the same errors again.

          Forgot mention that I completed the review and all sounds good to me.

          Show
          jacques.le.roux Jacques Le Roux added a comment - - edited 3. Right 4 Yep I thought so, just tried again with g cleanAll eclipse build "ofbiz -l readers=seed,demo,ext -l delegator=default -l group=org.apache.ofbiz" same errors than with java -jar build/libs/ofbiz.jar -l readers=seed,demo,ext 5. You mean that it's about the order (because loadAll works)? Of course using g cleanAll eclipse build "ofbiz -l readers=seed,demo,ext" I get the same errors again. Forgot mention that I completed the review and all sounds good to me.
          Hide
          taher Taher Alkhateeb added a comment -

          Hi Jacques, thank you for the review. Replying:

          1- yeah .. that sucks it's because of the time delay in committing this stuff. I will merge the conflicts and reattach
          2- Thank you .. will take care of it
          3- To remain consistent with the rest of the framework. Anyway, it makes almost no difference given the very small sizes of the data structures
          4- every "key=value" needs its own --load-data or -l. So the syntax should be ./gradlew "ofbiz -l readers=seed,demo,ext -l delegator=default -l group=org.apache.ofbiz"
          5- Why are using java -jar? just use ./gradlew and execute your commands to avoid confusion or misspelling of things. Anyway, it seems like the errors are not related to my work but rather improperly configured data. It's just a bunch of foreign key violations and things like that.

          Show
          taher Taher Alkhateeb added a comment - Hi Jacques, thank you for the review. Replying: 1- yeah .. that sucks it's because of the time delay in committing this stuff. I will merge the conflicts and reattach 2- Thank you .. will take care of it 3- To remain consistent with the rest of the framework. Anyway, it makes almost no difference given the very small sizes of the data structures 4- every "key=value" needs its own --load-data or -l. So the syntax should be ./gradlew "ofbiz -l readers=seed,demo,ext -l delegator=default -l group=org.apache.ofbiz" 5- Why are using java -jar? just use ./gradlew and execute your commands to avoid confusion or misspelling of things. Anyway, it seems like the errors are not related to my work but rather improperly configured data. It's just a bunch of foreign key violations and things like that.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          g cleanAll eclipse build loadAll

          works perfectly

          Show
          jacques.le.roux Jacques Le Roux added a comment - g cleanAll eclipse build loadAll works perfectly
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Hi Taher,
          I continued the review,

          1. I could not completely apply the changes README.md. I guess you need to update?
          2. found a typo

            Debug.logImportant("Creating foreign key indcies...", module);

          3. A question: in few places, why did you prefer ArrayList over LinkedList?
          4. Among
             -l,--load-data <key=value>   Creates tables/load data e.g:
                                          -l readers=seed,demo,ext
                                          -l file=/tmp/dataload1.xml,/tmp/dataload2.xml
                                          -l dir=directory/of/files
                                          -l component=base
                                          -l delegator=default
                                          -l group=org.apache.ofbiz
                                          -l timeout=7200
                                          -l create-pks
                                          -l drop-pks
                                          -l create-constraints
                                          -l drop-constraints
                                          -l create-fks
                                          -l maintain-txs
                                          -l try-inserts
                                          -l repair-columns
                                          -l continue-on-failure
            

            I tried to use several parameters (also with several combinations) at the same time w/o success, but not sure of the real syntax.
            Because using

            -l readers=seed,demo,ext delegator=default (etc.)

            did not work I used something like

            java -jar build/libs/ofbiz.jar -l readers=seed,demo,ext -l delegator=default group=org.apache.ofbiz (etc.)

          5. And finally while I tried (g is a shorcut for gradlew)

            C:\projectsASF\ofbiz>g cleanAll && g eclipse build && java -jar build/libs/ofbiz.jar -l readers=seed,demo,ext

            I got the attached log with errors at end. I have no pending changes in this instance but the patch here.

          Show
          jacques.le.roux Jacques Le Roux added a comment - Hi Taher, I continued the review, I could not completely apply the changes README.md. I guess you need to update? found a typo Debug.logImportant("Creating foreign key indcies...", module); A question: in few places, why did you prefer ArrayList over LinkedList? Among -l,--load-data <key=value> Creates tables/load data e.g: -l readers=seed,demo,ext -l file=/tmp/dataload1.xml,/tmp/dataload2.xml -l dir=directory/of/files -l component=base -l delegator= default -l group=org.apache.ofbiz -l timeout=7200 -l create-pks -l drop-pks -l create-constraints -l drop-constraints -l create-fks -l maintain-txs -l try -inserts -l repair-columns -l continue -on-failure I tried to use several parameters (also with several combinations) at the same time w/o success, but not sure of the real syntax. Because using -l readers=seed,demo,ext delegator=default (etc.) did not work I used something like java -jar build/libs/ofbiz.jar -l readers=seed,demo,ext -l delegator=default group=org.apache.ofbiz (etc.) And finally while I tried (g is a shorcut for gradlew) C:\projectsASF\ofbiz>g cleanAll && g eclipse build && java -jar build/libs/ofbiz.jar -l readers=seed,demo,ext I got the attached log with errors at end. I have no pending changes in this instance but the patch here.
          Hide
          taher Taher Alkhateeb added a comment -

          Thank you for your help, I'll wait for your review before committing. It's good to have a fresh pair of eyes.

          Show
          taher Taher Alkhateeb added a comment - Thank you for your help, I'll wait for your review before committing. It's good to have a fresh pair of eyes.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          1st, I like the README.md rewriting, I'll continue later...

          Show
          jacques.le.roux Jacques Le Roux added a comment - 1st, I like the README.md rewriting, I'll continue later...
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Ah we crossed on wire, good to know, thanks!

          Show
          jacques.le.roux Jacques Le Roux added a comment - Ah we crossed on wire, good to know, thanks!
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Oops, got it now, one name is not right. It should not be OFBIZ-9411.patch OK remains the last question

          Show
          jacques.le.roux Jacques Le Roux added a comment - Oops, got it now, one name is not right. It should not be OFBIZ-9411 .patch OK remains the last question
          Hide
          taher Taher Alkhateeb added a comment -

          Ignore all the patches, they are for illustration only. the latest patch does everything

          Show
          taher Taher Alkhateeb added a comment - Ignore all the patches, they are for illustration only. the latest patch does everything
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          HI Taher,

          I remember now, what refrained me to review is that it's not clear which is the last patch version.

          I understand there are 2 different patches

          1. OFBIZ-9411.patch
          2. OFBIZ-9441-fix-continue-on-failure.patch
            And that the last OFBIZ-9411.patch is from 21/Jul/17 16:14.

          But patches are weirdly grayed so I'm (a bit) unsure and especially I'd like to understand why it's so.

          Also is now the OFBIZ-9441-fix-continue-on-failure.patch included in the last OFBIZ-9411.patch?

          Thanks!

          Show
          jacques.le.roux Jacques Le Roux added a comment - HI Taher, I remember now, what refrained me to review is that it's not clear which is the last patch version. I understand there are 2 different patches OFBIZ-9411 .patch OFBIZ-9441 -fix-continue-on-failure.patch And that the last OFBIZ-9411 .patch is from 21/Jul/17 16:14. But patches are weirdly grayed so I'm (a bit) unsure and especially I'd like to understand why it's so. Also is now the OFBIZ-9441 -fix-continue-on-failure.patch included in the last OFBIZ-9411 .patch? Thanks!
          Hide
          taher Taher Alkhateeb added a comment -

          Attaching newest patch which in addition to everything above does:

          • Rename a few methods for better clarity
          • Minor code refactoring
          • Fixed a bug in which the filter stream for constructing the properties map is incorrect (I applied the filter in reverse). Now it works perfectly and I tested it with multiple different combos of commands on the command line. Essentially the bug was getting .equals() function against an enum which is wrong. It should be the enum .toString() instead.
          Show
          taher Taher Alkhateeb added a comment - Attaching newest patch which in addition to everything above does: Rename a few methods for better clarity Minor code refactoring Fixed a bug in which the filter stream for constructing the properties map is incorrect (I applied the filter in reverse). Now it works perfectly and I tested it with multiple different combos of commands on the command line. Essentially the bug was getting .equals() function against an enum which is wrong. It should be the enum .toString() instead.
          Hide
          taher Taher Alkhateeb added a comment -

          Attaching new patch that fixes incorrect behavior when passing the file or dir arguments. The code is now more solid

          Show
          taher Taher Alkhateeb added a comment - Attaching new patch that fixes incorrect behavior when passing the file or dir arguments. The code is now more solid
          Hide
          taher Taher Alkhateeb added a comment -

          Latest attachment fixes everything and works perfectly with "continue-on-failure" flag. I think this resolves the issue

          Show
          taher Taher Alkhateeb added a comment - Latest attachment fixes everything and works perfectly with "continue-on-failure" flag. I think this resolves the issue
          Hide
          taher Taher Alkhateeb added a comment -

          Okay, I figured out after a lot of digging what's going on. The culprit is the EntitySaxReader which is acting as the DefaultHandler for the input, and in the function StartElement it is swallowing an exception exactly here:

          try {
              currentValue = delegator.makeValue(entityName);
              // TODO: do we really want this? it makes it so none of the values imported have create/update timestamps set
              // DEJ 10/16/04 I think they should all be stamped, so commenting this out
              // JAZ 12/10/04 I think it should be specified when creating the reader
              if (this.maintainTxStamps) {
                  currentValue.setIsFromEntitySync(true);
              }
          } catch (Exception e) {
              Debug.logError(e, module);
          }
          

          I'm adding this attachment as a reference to where the problem exactly is and how I solved it. I will soon add a new patch that includes everything

          Show
          taher Taher Alkhateeb added a comment - Okay, I figured out after a lot of digging what's going on. The culprit is the EntitySaxReader which is acting as the DefaultHandler for the input, and in the function StartElement it is swallowing an exception exactly here: try { currentValue = delegator.makeValue(entityName); // TODO: do we really want this ? it makes it so none of the values imported have create/update timestamps set // DEJ 10/16/04 I think they should all be stamped, so commenting this out // JAZ 12/10/04 I think it should be specified when creating the reader if ( this .maintainTxStamps) { currentValue.setIsFromEntitySync( true ); } } catch (Exception e) { Debug.logError(e, module); } I'm adding this attachment as a reference to where the problem exactly is and how I solved it. I will soon add a new patch that includes everything
          Hide
          taher Taher Alkhateeb added a comment -

          Okay, so with this patch, the failure to load a data file does not crash the system. My investigation so far is showing that an exception is swallowed REALLY DEEP in the call stack. So I have to roll my sleeves and figure this out

          Show
          taher Taher Alkhateeb added a comment - Okay, so with this patch, the failure to load a data file does not crash the system. My investigation so far is showing that an exception is swallowed REALLY DEEP in the call stack. So I have to roll my sleeves and figure this out
          Hide
          taher Taher Alkhateeb added a comment -

          This is a major refactoring patch that applies the following changes:

          • Fully refactor both containers by breaking up the logic into many smaller
            methods which substantially improves the code readability.
          • Remove most old documentation and commented out code where applicable
          • Refactor both the test and data-load containers to reply on OFBiz commands
            instead of the old String array.
          • Delete the StratupCommandToArgsAdapter as it is no longer needed
          • Create a new feature in which OFBiz by default fails if any data file
            fails to load. This can be overridden by passing a flag called
            "continue-on-failure" e.g. ./gradlew "ofbiz --load-data continue-on-failure"
          • Add a new property to --test called "loglevel" (old code but working properly)
          • Add many new properties to the --load-data command including repair-columns,
            try-inserts, maintain-txs, etc ...
          • Update the documentation output of ./gradlew "ofbiz --help" to incorporate
            the new mentioned properties
          • Refactor README.md to incorporate the changes to the gradle commands for both
            "-load-data" and "-test" properties. Also remove the gradle GUI
            documentation as it is now deprecated.
          • Also refactor README.md in other locations to cleanup and make it more
            consistent. This icludes moving long notes into new sections and reducing
            the verbosity of the security header. Furthermore, created a new header
            called Miscellaneous documentation to house the newly created sections
          • lots of small changes to remove EOL white space

          This patch requires a lot of testing ,especially the use of the properties in
          different combinations to make sure all combinations work correctly. There were
          many properties for the --load-data command that are added but probably not
          used for a long time. I am not sure if this old code is relevant anymore, but I
          revived it anyway. Testing is the final judge on its merit.

          Show
          taher Taher Alkhateeb added a comment - This is a major refactoring patch that applies the following changes: Fully refactor both containers by breaking up the logic into many smaller methods which substantially improves the code readability. Remove most old documentation and commented out code where applicable Refactor both the test and data-load containers to reply on OFBiz commands instead of the old String array. Delete the StratupCommandToArgsAdapter as it is no longer needed Create a new feature in which OFBiz by default fails if any data file fails to load. This can be overridden by passing a flag called "continue-on-failure" e.g. ./gradlew "ofbiz --load-data continue-on-failure" Add a new property to --test called "loglevel" (old code but working properly) Add many new properties to the --load-data command including repair-columns, try-inserts, maintain-txs, etc ... Update the documentation output of ./gradlew "ofbiz --help" to incorporate the new mentioned properties Refactor README.md to incorporate the changes to the gradle commands for both "- load-data" and " -test" properties. Also remove the gradle GUI documentation as it is now deprecated. Also refactor README.md in other locations to cleanup and make it more consistent. This icludes moving long notes into new sections and reducing the verbosity of the security header. Furthermore, created a new header called Miscellaneous documentation to house the newly created sections lots of small changes to remove EOL white space This patch requires a lot of testing ,especially the use of the properties in different combinations to make sure all combinations work correctly. There were many properties for the --load-data command that are added but probably not used for a long time. I am not sure if this old code is relevant anymore, but I revived it anyway. Testing is the final judge on its merit.
          Hide
          taher Taher Alkhateeb added a comment - - edited

          This thread discusses and approves creating a new feature in which OFBiz fails by default if some data files are not loaded.

          Show
          taher Taher Alkhateeb added a comment - - edited This thread discusses and approves creating a new feature in which OFBiz fails by default if some data files are not loaded.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development