Details

    • Sprint:
      Re-Factor Sprint 1

      Description

      Looking at the main method and design of Start.java and the start component overall looks ugly. The things I would like to fix so far are:

      • the files are too long
      • some variables are not even needed (loaderArgs?)
      • the level of abstraction is wrong
      • main throws an exception!
      • the arguments processing logic is terrible, need to move it to commons-cli

      It's just so messy and ugly to look at. So for me refactoring starts at Start! Given that this is an important component, I will provide a patch to be reviewed by the community before committing just to be on the safe side.

      1. error.log
        8 kB
        Jacques Le Roux
      2. ofbiz.log
        188 kB
        Jacques Le Roux
      3. OFBIZ-6783.patch
        37 kB
        Taher Alkhateeb
      4. OFBIZ-6783.patch
        35 kB
        Taher Alkhateeb
      5. OFBIZ-6783.patch
        33 kB
        Taher Alkhateeb
      6. OFBIZ-6783.patch
        26 kB
        Taher Alkhateeb
      7. OFBIZ-6783.patch
        22 kB
        Taher Alkhateeb
      8. OFBIZ-6783.patch
        40 kB
        Taher Alkhateeb
      9. OFBIZ-6783.patch
        67 kB
        Taher Alkhateeb
      10. OFBIZ-6783.patch
        63 kB
        Taher Alkhateeb
      11. OFBIZ-6783.patch
        63 kB
        Taher Alkhateeb
      12. OFBIZ-6783.patch
        44 kB
        Taher Alkhateeb
      13. OFBIZ-6783.patch
        53 kB
        Taher Alkhateeb
      14. OFBIZ-6783.patch
        52 kB
        Taher Alkhateeb
      15. OFBIZ-6783.patch
        53 kB
        Taher Alkhateeb
      16. OFBIZ-6783.patch
        52 kB
        Taher Alkhateeb
      17. OFBIZ-6783.patch
        51 kB
        Taher Alkhateeb
      18. OFBIZ-6783.patch
        52 kB
        Taher Alkhateeb
      19. OFBIZ-6783.patch
        52 kB
        Taher Alkhateeb
      20. OFBIZ-6783.patch
        20 kB
        Taher Alkhateeb
      21. OFBIZ-6783.patch
        14 kB
        Taher Alkhateeb
      22. OFBIZ-6783-hiddenfiles.patch
        1 kB
        Jacopo Cappellato
      23. StartCommandUtil.java
        5 kB
        Taher Alkhateeb

        Issue Links

          Activity

          Hide
          taher Taher Alkhateeb added a comment -

          Hey everyone, please check this patch, I've done the following so far:

          • remove private comments relating to the class to the JavaDoc location at the top of the class
          • relocate class fields to the top of the class as per Java conventions
          • relocate the constructor with its singleton instance to the top of the class below the fields
          • sort all the methods starting with main, followed by public, default and private
          • add JavaDoc to main
          • remove System.exit(99) from main and replace it with throw new StartException(e); I did this because main declares that it throws a StartupException which it never does
          • breakup the logic part to evaluate ofbiz command to a new method called evaluateOfbizCommand(String[] args). I will replace this eventually with apache commons-cli
          • make startStartLoaders private, since it is not called outside Start.java
          Show
          taher Taher Alkhateeb added a comment - Hey everyone, please check this patch, I've done the following so far: remove private comments relating to the class to the JavaDoc location at the top of the class relocate class fields to the top of the class as per Java conventions relocate the constructor with its singleton instance to the top of the class below the fields sort all the methods starting with main, followed by public, default and private add JavaDoc to main remove System.exit(99) from main and replace it with throw new StartException(e); I did this because main declares that it throws a StartupException which it never does breakup the logic part to evaluate ofbiz command to a new method called evaluateOfbizCommand(String[] args). I will replace this eventually with apache commons-cli make startStartLoaders private, since it is not called outside Start.java
          Hide
          taher Taher Alkhateeb added a comment -

          I also forgot to mention that I deleted the list loaderArgs declared inside main, it is simply not used for any purpose

          Show
          taher Taher Alkhateeb added a comment - I also forgot to mention that I deleted the list loaderArgs declared inside main, it is simply not used for any purpose
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Hi Taher,

          Thanks for the detailled explanations. After review and few tests, all your changes are OK with me. I agree about using commons-cli

          Show
          jacques.le.roux Jacques Le Roux added a comment - Hi Taher, Thanks for the detailled explanations. After review and few tests, all your changes are OK with me. I agree about using commons-cli
          Hide
          jacopoc Jacopo Cappellato added a comment -

          Looks good.

          Show
          jacopoc Jacopo Cappellato added a comment - Looks good.
          Hide
          taher Taher Alkhateeb added a comment -

          Thank you gentlemen. Patch is committed in r1741624.

          The next items I have in mind for this JIRA is the following:

          • To introduce commons-cli. I will keep it in the base component but refer to it from start
          • To remove dependencies from other classes into Start.java. NOTHING should point to Start.java
          • I am thinking of creating an OfbizCommand interface for executing the commands. Start.java should be a very small high level execution of the framework with all details hidden away.
          • some code need to be simplified
          Show
          taher Taher Alkhateeb added a comment - Thank you gentlemen. Patch is committed in r1741624. The next items I have in mind for this JIRA is the following: To introduce commons-cli. I will keep it in the base component but refer to it from start To remove dependencies from other classes into Start.java. NOTHING should point to Start.java I am thinking of creating an OfbizCommand interface for executing the commands. Start.java should be a very small high level execution of the framework with all details hidden away. some code need to be simplified
          Hide
          jacopoc Jacopo Cappellato added a comment -

          Hi Taher,

          I am not sure about the first point since the start component has been built to be independent (or sort of) from the base component... we may want to maintain this and have commons-cli in the start component.

          Show
          jacopoc Jacopo Cappellato added a comment - Hi Taher, I am not sure about the first point since the start component has been built to be independent (or sort of) from the base component... we may want to maintain this and have commons-cli in the start component.
          Hide
          taher Taher Alkhateeb added a comment -

          Hi Jacopo,

          I was thinking that something else might need the commons-cli in the framework and so preferred keeping it in base. But anyway, no problem I can keep it in start. Either way, I need to modify start's build.xml to include the class-path in the jar construction otherwise it will not work (because ofbiz.jar is copied to root)

          Since you opened this subject up anyway, the radical idea I have in mind is to actually (more or less) eliminate the idea of components entirely in /framework. I think the framework should be one independent thing that loads other components. Anyway, I will compile my thoughts on this and discuss it at a later stage on the mailing list in the future.

          Show
          taher Taher Alkhateeb added a comment - Hi Jacopo, I was thinking that something else might need the commons-cli in the framework and so preferred keeping it in base. But anyway, no problem I can keep it in start. Either way, I need to modify start's build.xml to include the class-path in the jar construction otherwise it will not work (because ofbiz.jar is copied to root) Since you opened this subject up anyway, the radical idea I have in mind is to actually (more or less) eliminate the idea of components entirely in /framework. I think the framework should be one independent thing that loads other components. Anyway, I will compile my thoughts on this and discuss it at a later stage on the mailing list in the future.
          Hide
          taher Taher Alkhateeb added a comment -

          I have been working non-stop for over a week trying to refactor Start.java. The code is absolutely disgusting with amazingly dumb design decisions!

          First, the StartupLoader constructor takes an argument "String[] args", which means that args that are passed to main, are just propagated down to every single container in the system, and every single container would have to parse the args, remove dashes, and process them!! Oh my god, this is absolutely sh$##y

          So I'm trying to replace the args with commons-cli, and I trace down what breaks, and EVERYTHING breaks. Here is my investigation so far:

          • Start.init(...) breaks, so I have to refactor that
          • Config breaks as it handles args, so I have to refactor that
          • The loaderArgs list in Start.java is actually copied the values from args, which is then passed to the ContainerLoader in the line loader.load(config, argsArray)
          • the ContainerLoader has this terrible piece of code inside a loop:
            Container tmpContainer = loadContainer(containerCfg, args);
            
          • That crap code eventually calls this code also inside the "loadContainer" method:
            containerObj.init(args, containerCfg.name, configFile);
          • Finally, every single container in the system (16 listed below) gets a copy of the raw unprocessed args
          • Of the 16 containers, four containers actually use the args as follows:
            • EntityDataLoadContainer
            • TestListContainer
            • TestRunContainer
            • XuiContainer
          • All system containers:
          • BeanShellContainer.java
          • ComponentContainer.java
          • JustLoadComponentsContainer.java
          • NamingServiceContainer.java
          • DelegatorContainer.java
          • GeronimoContainer.java
          • JavaMailContainer.java
          • ServiceContainer.java
          • RmiServiceContainer.java
          • EntityDataLoadContainer.java
          • CatalinaContainer.java
          • TestListContainer.java
          • TestRunContainer.java
          • BirtContainer.java
          • XuiContainer.java
          • JposDeviceContainer.java

          So to fix all the above we must apply the following:

          • Change the signature of the containers not to include args
          • Pass the args in a different format after processing
          • Fix everything everywhere
          Show
          taher Taher Alkhateeb added a comment - I have been working non-stop for over a week trying to refactor Start.java. The code is absolutely disgusting with amazingly dumb design decisions! First, the StartupLoader constructor takes an argument "String[] args", which means that args that are passed to main, are just propagated down to every single container in the system, and every single container would have to parse the args, remove dashes, and process them!! Oh my god, this is absolutely sh$##y So I'm trying to replace the args with commons-cli, and I trace down what breaks, and EVERYTHING breaks. Here is my investigation so far: Start.init(...) breaks, so I have to refactor that Config breaks as it handles args, so I have to refactor that The loaderArgs list in Start.java is actually copied the values from args, which is then passed to the ContainerLoader in the line loader.load(config, argsArray) the ContainerLoader has this terrible piece of code inside a loop: Container tmpContainer = loadContainer(containerCfg, args); That crap code eventually calls this code also inside the "loadContainer" method: containerObj.init(args, containerCfg.name, configFile); Finally, every single container in the system (16 listed below) gets a copy of the raw unprocessed args Of the 16 containers, four containers actually use the args as follows: EntityDataLoadContainer TestListContainer TestRunContainer XuiContainer All system containers: BeanShellContainer.java ComponentContainer.java JustLoadComponentsContainer.java NamingServiceContainer.java DelegatorContainer.java GeronimoContainer.java JavaMailContainer.java ServiceContainer.java RmiServiceContainer.java EntityDataLoadContainer.java CatalinaContainer.java TestListContainer.java TestRunContainer.java BirtContainer.java XuiContainer.java JposDeviceContainer.java So to fix all the above we must apply the following: Change the signature of the containers not to include args Pass the args in a different format after processing Fix everything everywhere
          Hide
          taher Taher Alkhateeb added a comment -

          This is my work so far in trying to resolve the problem, If you would like to help, don't forget to download the commons library and put it in the correct path of framework/start/lib for testing to work

          Show
          taher Taher Alkhateeb added a comment - This is my work so far in trying to resolve the problem, If you would like to help, don't forget to download the commons library and put it in the correct path of framework/start/lib for testing to work
          Hide
          taher Taher Alkhateeb added a comment -

          I just realized my patch is missing a new file, so here goes

          Show
          taher Taher Alkhateeb added a comment - I just realized my patch is missing a new file, so here goes
          Hide
          taher Taher Alkhateeb added a comment -

          After a lot of work, I finally have a working patch that passes all tests and runs ant commands correctly.

          This is a big patch that does the following:

          • adds the commons-cli to the classpath
          • fixes the start component build.xml to include commons-cli
          • change the formatting of all commands in java -jar ofbiz.jar to be unified in the way they are used
          • provide much better printing when doing java -jar ofbiz.jar --help
          • fix the main build.xml to follow the new syntax
          • replace the raw args that are passed everywhere in the ofbiz with a more specialized StartupCommand.java object. This completely decouples the processing of arguments from ofbiz
          • also, hide all commons-cli operation in a class called StartupCommandUtil. This keeps ofbiz decoupled from commons-cli.
          • Unify the exception model as much as possible to be based on the StartupException
          • simplify Start.main and Start.init
          • Because args propagate everywhere, I had to create an adapter method called populateLoaderArgs that builds args as expected by the containers. This allows us to change the ofbiz arguments without breaking the system

          To test this patch:

          All the tests work, however, a lot of testing is needed on almost all the build.xml targets to ensure that processing is done correctly. Also, testing all kinds of scenarios on java -jar ofbiz.jar

          Show
          taher Taher Alkhateeb added a comment - After a lot of work, I finally have a working patch that passes all tests and runs ant commands correctly. This is a big patch that does the following: adds the commons-cli to the classpath fixes the start component build.xml to include commons-cli change the formatting of all commands in java -jar ofbiz.jar to be unified in the way they are used provide much better printing when doing java -jar ofbiz.jar --help fix the main build.xml to follow the new syntax replace the raw args that are passed everywhere in the ofbiz with a more specialized StartupCommand.java object. This completely decouples the processing of arguments from ofbiz also, hide all commons-cli operation in a class called StartupCommandUtil. This keeps ofbiz decoupled from commons-cli. Unify the exception model as much as possible to be based on the StartupException simplify Start.main and Start.init Because args propagate everywhere, I had to create an adapter method called populateLoaderArgs that builds args as expected by the containers. This allows us to change the ofbiz arguments without breaking the system To test this patch: apply the patch download and copy the commons-cli jar to framework/start/lib/commons-cli-1.3.1.jar. You can find it in http://www-us.apache.org/dist//commons/cli/binaries/commons-cli-1.3.1-bin.zip ./ant clean-all load-demo run-tests All the tests work, however, a lot of testing is needed on almost all the build.xml targets to ensure that processing is done correctly. Also, testing all kinds of scenarios on java -jar ofbiz.jar
          Hide
          jacques.le.roux Jacques Le Roux added a comment - - edited

          Hi Taher,

          Nice, so far, all seems to work smoothly, I will keep it running locally...

          Show
          jacques.le.roux Jacques Le Roux added a comment - - edited Hi Taher, Nice, so far, all seems to work smoothly, I will keep it running locally...
          Hide
          jacques.le.roux Jacques Le Roux added a comment - - edited

          I used the portoffset with success

          Show
          jacques.le.roux Jacques Le Roux added a comment - - edited I used the portoffset with success
          Hide
          jacques.le.roux Jacques Le Roux added a comment - - edited

          "Pos" and "Both" options work well as well.
          It seems you can't use "both" option with debugging. But this was also not possible before... (I tried)

          java -jar ofbiz.jar -b -g
          

          Ha sorry just read "note: Only one command can execute at a time. Portoffset may be appended" so it's OK for

          java -jar ofbiz.jar -b -g
          

          which is wrong

          Show
          jacques.le.roux Jacques Le Roux added a comment - - edited "Pos" and "Both" options work well as well. It seems you can't use "both" option with debugging. But this was also not possible before... (I tried) java -jar ofbiz.jar -b -g Ha sorry just read "note: Only one command can execute at a time. Portoffset may be appended" so it's OK for java -jar ofbiz.jar -b -g which is wrong
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Using "java -jar ofbiz.jar -g"
          got "Failed to connect to remote VM. Connection refused. Connection refused: connect" with Eclipse

          Show
          jacques.le.roux Jacques Le Roux added a comment - Using "java -jar ofbiz.jar -g" got "Failed to connect to remote VM. Connection refused. Connection refused: connect" with Eclipse
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          "java -jar ofbiz.jar -s" works well

          Show
          jacques.le.roux Jacques Le Roux added a comment - "java -jar ofbiz.jar -s" works well
          Hide
          taher Taher Alkhateeb added a comment -

          okay, I double checked the failed connection to the VM, essentially this has nothing to do with my logic, but you need to pass args directly to the VM not the application.

          So it should be java -Xrunjdwp:transport=dt_socket,server=y,suspend=n,address=8091 -jar ofbiz.jar --debug

          Also, I discovered two simple mistakes in the main build.xml, --start-debug should be --debug and --start-batch should be --batch. I fixed both and will attach another patch later

          Show
          taher Taher Alkhateeb added a comment - okay, I double checked the failed connection to the VM, essentially this has nothing to do with my logic, but you need to pass args directly to the VM not the application. So it should be java -Xrunjdwp:transport=dt_socket,server=y,suspend=n,address=8091 -jar ofbiz.jar --debug Also, I discovered two simple mistakes in the main build.xml, --start-debug should be --debug and --start-batch should be --batch. I fixed both and will attach another patch later
          Hide
          taher Taher Alkhateeb added a comment -

          Hi Jacque,

          I don't see it ok at my side! running java -jar ofbiz.jar -b -g does not work and yield the following message:
          Error: The option 'g' was specified but an option from this group has already been selected: 'b'
          Are you sure it worked?

          Show
          taher Taher Alkhateeb added a comment - Hi Jacque, I don't see it ok at my side! running java -jar ofbiz.jar -b -g does not work and yield the following message: Error: The option 'g' was specified but an option from this group has already been selected: 'b' Are you sure it worked?
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Nope, I meant it does not work, but it's ok since the note explains it. BTW you also can't do that with any ant target at the moment. Anyway, if ever someone needs that s/he will contribute it, I guess...

          Show
          jacques.le.roux Jacques Le Roux added a comment - Nope, I meant it does not work, but it's ok since the note explains it. BTW you also can't do that with any ant target at the moment. Anyway, if ever someone needs that s/he will contribute it, I guess...
          Hide
          jacques.le.roux Jacques Le Roux added a comment - - edited

          I tried to run "java -jar ofbiz.jar -t" on Win7. It ran, but did not stop, nor created the HTML results. This could be related (for the non-stop part)

          2016-05-11 20:56:31,257 |Thread-0             |ContainerLoader               |I| Stopped container testtools-container
          2016-05-11 20:56:31,257 |Thread-0             |ContainerLoader               |I| Stopping container catalina-container-test
          2016-05-11 20:56:31,480 FATAL Unable to register shutdown hook because JVM is shutting down.
          
          Show
          jacques.le.roux Jacques Le Roux added a comment - - edited I tried to run "java -jar ofbiz.jar -t" on Win7. It ran, but did not stop, nor created the HTML results. This could be related (for the non-stop part) 2016-05-11 20:56:31,257 | Thread -0 |ContainerLoader |I| Stopped container testtools-container 2016-05-11 20:56:31,257 | Thread -0 |ContainerLoader |I| Stopping container catalina-container-test 2016-05-11 20:56:31,480 FATAL Unable to register shutdown hook because JVM is shutting down.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          About

          java -Xrunjdwp:transport=dt_socket,server=y,suspend=n,address=8091 -jar ofbiz.jar --debug

          So should not the help documentation be amended? (I will still prefer ant start-debug )

          Show
          jacques.le.roux Jacques Le Roux added a comment - About java -Xrunjdwp:transport=dt_socket,server=y,suspend=n,address=8091 -jar ofbiz.jar --debug So should not the help documentation be amended? (I will still prefer ant start-debug )
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Forget it, this seems unrelated with your changes. I get the same when using ant in a not patched WC.

          Show
          jacques.le.roux Jacques Le Roux added a comment - Forget it, this seems unrelated with your changes. I get the same when using ant in a not patched WC.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -
          Show
          jacques.le.roux Jacques Le Roux added a comment - See OFBIZ-7065
          Hide
          taher Taher Alkhateeb added a comment -

          great, it seems this is a pretty stable patch. Better than I expected

          regarding ant targets, i'm not renaming them, the args to them are wrong. i did not rename any ant target.

          Testing the ant targets is the next step, if all is good I think we can commit

          Show
          taher Taher Alkhateeb added a comment - great, it seems this is a pretty stable patch. Better than I expected regarding ant targets, i'm not renaming them, the args to them are wrong. i did not rename any ant target. Testing the ant targets is the next step, if all is good I think we can commit
          Hide
          taher Taher Alkhateeb added a comment -

          fixing this is one line of code thanks to commons-cli and the way StartupCommandUtil is designed. Very trivial as far as args processing is concerned. The problem is making sure they play nice toghether.

          Show
          taher Taher Alkhateeb added a comment - fixing this is one line of code thanks to commons-cli and the way StartupCommandUtil is designed. Very trivial as far as args processing is concerned. The problem is making sure they play nice toghether.
          Hide
          taher Taher Alkhateeb added a comment -

          hmmm yes perhaps we need to mention that the jvm args must be passed. i'll update the docs along with the incorrect args in the next patch

          Show
          taher Taher Alkhateeb added a comment - hmmm yes perhaps we need to mention that the jvm args must be passed. i'll update the docs along with the incorrect args in the next patch
          Hide
          hansbak Hans Bakker added a comment -

          wouldn't it be nice (or even required?) to write an automated jUnit test for this new start.java?

          Show
          hansbak Hans Bakker added a comment - wouldn't it be nice (or even required?) to write an automated jUnit test for this new start.java?
          Hide
          taher Taher Alkhateeb added a comment -

          Hi Hans,

          I'm glad you're participating. FYI This is becoming more than just refactoring start as I'm also changing Config, CommonsDaemonStart and 2 build.xml files.

          Anyway, back to your question, I'm a guy who's acquired TDD down to my bones. However, testing the changes here pose multiple challenges and I appreciate your feedback on them:

          • The code is still horrible and messy and everything jumbled together. So I can not do isolated unit tests in most places. I still need to refactor and breakdown the code some more to make it clean and testable.
          • The start component does not allow for testing (it's bootstrapping code). So the only way I can test anything is from another component (say, base).
          • The magic work of commons-cli is all happening in StartupCommandUtil.parseOfbizCommands(args). However, I declared it package protected. If I want to test it, then I have to declare it public, and I'm trying as much as I can to isolate start from the rest of the framework.

          I worked hard to decouple ofbiz from both the command line arguments and also from commons-cli. Start is a special component in which it is not easy to test. In fact, it was really hard work to change build.xml to make it work with the commons-cli jar.

          So in summary, I'm not sure what the best course of action is, but I feel exposing my methods as public only for the purpose of testing is not a pretty solution. The same can also said about reflection.

          Maybe I can create a public adapter class or something, I'm not sure. Do you have any thoughts?

          Show
          taher Taher Alkhateeb added a comment - Hi Hans, I'm glad you're participating. FYI This is becoming more than just refactoring start as I'm also changing Config, CommonsDaemonStart and 2 build.xml files. Anyway, back to your question, I'm a guy who's acquired TDD down to my bones. However, testing the changes here pose multiple challenges and I appreciate your feedback on them: The code is still horrible and messy and everything jumbled together. So I can not do isolated unit tests in most places. I still need to refactor and breakdown the code some more to make it clean and testable. The start component does not allow for testing (it's bootstrapping code). So the only way I can test anything is from another component (say, base). The magic work of commons-cli is all happening in StartupCommandUtil.parseOfbizCommands(args). However, I declared it package protected. If I want to test it, then I have to declare it public, and I'm trying as much as I can to isolate start from the rest of the framework. I worked hard to decouple ofbiz from both the command line arguments and also from commons-cli. Start is a special component in which it is not easy to test. In fact, it was really hard work to change build.xml to make it work with the commons-cli jar. So in summary, I'm not sure what the best course of action is, but I feel exposing my methods as public only for the purpose of testing is not a pretty solution. The same can also said about reflection. Maybe I can create a public adapter class or something, I'm not sure. Do you have any thoughts?
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          i'm not renaming them, the args to them are wrong. i did not rename any ant target.

          What do you mean by that exactly?

          i did not rename any ant target.

          I know you did not, it's unrelated because I get it with an up todate and unpatched local Working Copy

          Show
          jacques.le.roux Jacques Le Roux added a comment - i'm not renaming them, the args to them are wrong. i did not rename any ant target. What do you mean by that exactly? i did not rename any ant target. I know you did not, it's unrelated because I get it with an up todate and unpatched local Working Copy
          Hide
          taher Taher Alkhateeb added a comment -

          Oh sorry for the confusion, you did not understand me because I'm talking about an earlier discussion. Let me make it clear below

          If you look at our conversation above, you said "I will still prefer ant start-debug". And if you look at the new build.xml which I'm going to send, it will be exactly like you want it i.e. ant start-debug. I'm not changing ant start-debug to ant debug. Instead what I'm changing is the arguments inside the ant target start-debug. Specifically, I will change the argument --start-debug to be --debug (which is passed to ofbiz.jar).

          I hope this makes clear for you now. In short, all the ant targets are going to remain the same. The only thing that changes is the arguments inside of them to comply with the new requirements of jara -jar ofbiz.jar

          Show
          taher Taher Alkhateeb added a comment - Oh sorry for the confusion, you did not understand me because I'm talking about an earlier discussion. Let me make it clear below If you look at our conversation above, you said "I will still prefer ant start-debug". And if you look at the new build.xml which I'm going to send, it will be exactly like you want it i.e. ant start-debug. I'm not changing ant start-debug to ant debug. Instead what I'm changing is the arguments inside the ant target start-debug. Specifically, I will change the argument --start-debug to be --debug (which is passed to ofbiz.jar). I hope this makes clear for you now. In short, all the ant targets are going to remain the same. The only thing that changes is the arguments inside of them to comply with the new requirements of jara -jar ofbiz.jar
          Hide
          taher Taher Alkhateeb added a comment -

          ok, this is the latest patch which adds the following to the previous patch

          • fixes the incorrect target arguments --start-debug and --start-batch
          • updates the documentation of java -jar ofbiz.jar --debug
          Show
          taher Taher Alkhateeb added a comment - ok, this is the latest patch which adds the following to the previous patch fixes the incorrect target arguments --start-debug and --start-batch updates the documentation of java -jar ofbiz.jar --debug
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          OK, clear thanks. BTW what I mean is I will still prefer
          "ant start-debug"
          over
          "java -Xrunjdwp:transport=dt_socket,server=y,suspend=n,address=8091 -jar ofbiz.jar --debug"
          see?

          Show
          jacques.le.roux Jacques Le Roux added a comment - OK, clear thanks. BTW what I mean is I will still prefer "ant start-debug" over "java -Xrunjdwp:transport=dt_socket,server=y,suspend=n,address=8091 -jar ofbiz.jar --debug" see?
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Oh, I see you last comment, I need to test that

          Show
          jacques.le.roux Jacques Le Roux added a comment - Oh, I see you last comment, I need to test that
          Hide
          taher Taher Alkhateeb added a comment -

          ok yes I get it now.

          It's unfortunate that I cannot clean it up, but those are JVM arguments, so I cannot do anything about it and like you I also prefer ant start-debug

          Still, I would say probably java -jar ofbiz.jar is now much, much better and prettier with these fixes. It used to be totally out of sync with the actual options being passed, documentation missing, and inconsistent commands (some take dashes and some don't)

          Show
          taher Taher Alkhateeb added a comment - ok yes I get it now. It's unfortunate that I cannot clean it up, but those are JVM arguments, so I cannot do anything about it and like you I also prefer ant start-debug Still, I would say probably java -jar ofbiz.jar is now much, much better and prettier with these fixes. It used to be totally out of sync with the actual options being passed, documentation missing, and inconsistent commands (some take dashes and some don't)
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          I totally agree, it's far better. When it will be committed, I'll update the wiki documentation. I mean at least that

          But before, we still need to check things of course... Should not be too long...

          Show
          jacques.le.roux Jacques Le Roux added a comment - I totally agree, it's far better. When it will be committed, I'll update the wiki documentation. I mean at least that But before, we still need to check things of course... Should not be too long...
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Then we can also close OFBIZ-5872

          Show
          jacques.le.roux Jacques Le Roux added a comment - Then we can also close OFBIZ-5872
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          I see no difference between
          java -jar ofbiz.jar --batch
          and
          java -jar ofbiz.jar --start

          This is what you get when you use
          ant start-batch
          on Win7

          C:\projectASF-Mars\ofbiz>ant start-batch
          "C:\Program Files\Java\jdk1.8.0_74\bin\java" -jar "C:\projectASF-Mars\ofbiz\framework\base\lib\ant-1.9.0-ant-launcher.jar" -lib "C:\projectASF-Mars\ofbiz\framework\base\lib\ant" start-batch
          Buildfile: C:\projectASF-Mars\ofbiz\build.xml
          
          start-batch:
          
          BUILD SUCCESSFUL
          Total time: 1 second
          
          Show
          jacques.le.roux Jacques Le Roux added a comment - I see no difference between java -jar ofbiz.jar --batch and java -jar ofbiz.jar --start This is what you get when you use ant start-batch on Win7 C:\projectASF-Mars\ofbiz>ant start-batch "C:\Program Files\Java\jdk1.8.0_74\bin\java" -jar "C:\projectASF-Mars\ofbiz\framework\base\lib\ant-1.9.0-ant-launcher.jar" -lib "C:\projectASF-Mars\ofbiz\framework\base\lib\ant" start-batch Buildfile: C:\projectASF-Mars\ofbiz\build.xml start-batch: BUILD SUCCESSFUL Total time: 1 second
          Hide
          jacques.le.roux Jacques Le Roux added a comment - - edited

          All options work for me, but --batch (see comment above)
          Also I'm not sure how to use "--testlist".
          I mean which kind of file should I put after "--testlist file=" ?

          Show
          jacques.le.roux Jacques Le Roux added a comment - - edited All options work for me, but --batch (see comment above) Also I'm not sure how to use "--testlist". I mean which kind of file should I put after "--testlist file=" ?
          Hide
          taher Taher Alkhateeb added a comment -

          You can find anexample in build.xml. This feature has something to do with webtools. The only reason I included it is because it is already there. You just need to make sure that the correct container is fired upon running this

          Show
          taher Taher Alkhateeb added a comment - You can find anexample in build.xml. This feature has something to do with webtools. The only reason I included it is because it is already there. You just need to make sure that the correct container is fired upon running this
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          An example, sorry which one?

          Show
          jacques.le.roux Jacques Le Roux added a comment - An example, sorry which one?
          Hide
          taher Taher Alkhateeb added a comment -
               <target name="_setup-separated-test-run" depends="_check-separated-tests-already-setup" unless="_separated-tests-already-setup">
                   <java jar="ofbiz.jar" fork="true">
                       <jvmarg value="${memory.initial.param}"/>
                       <jvmarg value="${memory.max.param}"/>
                       <arg value="--testlist"/>
                       <arg value="file=runtime/test-list-build.xml"/>
                       <arg value="--testlist"/>
                       <arg value="mode=ant"/>
                       <env key="LC_ALL" value="C"/>
                   </java>
               </target>
          

          As for the start-batch issue. I did not understand exactly what you faced? and how is it different from pre-patch?

          Show
          taher Taher Alkhateeb added a comment - <target name= "_setup-separated-test-run" depends= "_check-separated-tests-already-setup" unless= "_separated-tests-already-setup" > <java jar= "ofbiz.jar" fork= "true" > <jvmarg value= "${memory.initial.param}" /> <jvmarg value= "${memory.max.param}" /> <arg value= "--testlist" /> <arg value= "file=runtime/test-list-build.xml" /> <arg value= "--testlist" /> <arg value= "mode=ant" /> <env key= "LC_ALL" value= "C" /> </java> </target> As for the start-batch issue. I did not understand exactly what you faced? and how is it different from pre-patch?
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          When you run
          ant start-batch
          you get what I posted above

          When you run
          java -jar ofbiz.jar --batch
          you get the same than when you run
          java -jar ofbiz.jar --start

          In other words it's not a batch in background

          Show
          jacques.le.roux Jacques Le Roux added a comment - When you run ant start-batch you get what I posted above When you run java -jar ofbiz.jar --batch you get the same than when you run java -jar ofbiz.jar --start In other words it's not a batch in background
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          I now see what you mean by
          java -jar ofbiz.jar --testlist
          But I still wonder why you put it there and if it will be useful at all (I mean used). I guess it needs some preparation and this is not obvious with the short documentation given by
          java -jar ofbiz.jar ?

          In other words I wonder if it does not bring more confusion than anything else I'm an adept of the KISS and YAGNI ways!

          Show
          jacques.le.roux Jacques Le Roux added a comment - I now see what you mean by java -jar ofbiz.jar --testlist But I still wonder why you put it there and if it will be useful at all (I mean used). I guess it needs some preparation and this is not obvious with the short documentation given by java -jar ofbiz.jar ? In other words I wonder if it does not bring more confusion than anything else I'm an adept of the KISS and YAGNI ways!
          Hide
          taher Taher Alkhateeb added a comment -

          for me personally, delete is the best key on the keyboard. However testlist is more than just a command. there is a full ofbiz container for it along with a properties file. I did not dig into that code yet nor do I understand the impact of removing it. So i'm not sure what to do but that is why I added it in the list of commands. I think it has something to do with webtools but not sure

          Show
          taher Taher Alkhateeb added a comment - for me personally, delete is the best key on the keyboard. However testlist is more than just a command. there is a full ofbiz container for it along with a properties file. I did not dig into that code yet nor do I understand the impact of removing it. So i'm not sure what to do but that is why I added it in the list of commands. I think it has something to do with webtools but not sure
          Hide
          taher Taher Alkhateeb added a comment -

          is this behavior different than pre patch?

          Show
          taher Taher Alkhateeb added a comment - is this behavior different than pre patch?
          Hide
          taher Taher Alkhateeb added a comment -

          Hi jacques, after further checking on the code, I suspect that start-batch and start-debug are both identical to start. The difference really is in the way it is called from ant. For example, in start-debug the real difference is in the JVM args passed, and the real difference in start-batch is in the spawn="true" ant attribute which spawns a new process for executing ofbiz.

          It seems my refactoring is bringing to light a lot of old abandoned and ugly code. This just shows you how extremely nasty the old code is.

          Anyway, I am inclined initially to delete the --batch and --debug targets completely both from build.xml and from Java code while keeping the ant targets but changing the argument to --start for both of them.

          Any thoughts?

          Show
          taher Taher Alkhateeb added a comment - Hi jacques, after further checking on the code, I suspect that start-batch and start-debug are both identical to start. The difference really is in the way it is called from ant. For example, in start-debug the real difference is in the JVM args passed, and the real difference in start-batch is in the spawn="true" ant attribute which spawns a new process for executing ofbiz. It seems my refactoring is bringing to light a lot of old abandoned and ugly code. This just shows you how extremely nasty the old code is. Anyway, I am inclined initially to delete the --batch and --debug targets completely both from build.xml and from Java code while keeping the ant targets but changing the argument to --start for both of them. Any thoughts?
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          +1

          Show
          jacques.le.roux Jacques Le Roux added a comment - +1
          Hide
          taher Taher Alkhateeb added a comment -

          ok, this patch fixes everything discussed previously plus removes the targets --batch and --debug given that they are really issued from ant, not from ofbiz.jar directly.

          Show
          taher Taher Alkhateeb added a comment - ok, this patch fixes everything discussed previously plus removes the targets --batch and --debug given that they are really issued from ant, not from ofbiz.jar directly.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Though I did not test the last patch, for me it's OK to commit it if the only changes are the targets --batch and --debug removed.

          I'd though have appreciated more explanations about the --testlist option. This may be covered in the wiki documentation (see link above)...

          Show
          jacques.le.roux Jacques Le Roux added a comment - Though I did not test the last patch, for me it's OK to commit it if the only changes are the targets --batch and --debug removed. I'd though have appreciated more explanations about the --testlist option. This may be covered in the wiki documentation (see link above)...
          Hide
          jacques.le.roux Jacques Le Roux added a comment - - edited

          OK, I compared the 2 last patches, it's OK with me!

          Show
          jacques.le.roux Jacques Le Roux added a comment - - edited OK, I compared the 2 last patches, it's OK with me!
          Hide
          taher Taher Alkhateeb added a comment -

          Hi Jacques,

          Okay I investigated a bit more and I think I understand now what --testlist does.

          The --testlist essentially runs a list of tests that are configured in a text file or an ant file. I am not exactly sure of the format but this is what I understood so far.

          So, I will update the documentation accordingly and provide a final patch for your review. If you concur I will commit.

          Show
          taher Taher Alkhateeb added a comment - Hi Jacques, Okay I investigated a bit more and I think I understand now what --testlist does. The --testlist essentially runs a list of tests that are configured in a text file or an ant file. I am not exactly sure of the format but this is what I understood so far. So, I will update the documentation accordingly and provide a final patch for your review. If you concur I will commit.
          Hide
          taher Taher Alkhateeb added a comment -

          No, I was wrong. --testlist, now I believe the --testlist actually outputs all ofbiz tests to either a text file or an ant build file.

          This generated file will be consumed by other targets, namely:

          • run-test-list: it uses the generated ant script to run all tests, but stopping ofbiz in between each test
          • cobertura-report-xml: not sure how, it's a macro and I need to investigate it.

          So, --testlist is rarely used, and I'm not sure what the value of it is. I think the best thing to do for now is to update the documentation to say something like "generates an script or text file containing a list of tests". Also, I discovered some errors which I will correct in the next patch

          Show
          taher Taher Alkhateeb added a comment - No, I was wrong. --testlist, now I believe the --testlist actually outputs all ofbiz tests to either a text file or an ant build file. This generated file will be consumed by other targets, namely: run-test-list: it uses the generated ant script to run all tests, but stopping ofbiz in between each test cobertura-report-xml: not sure how, it's a macro and I need to investigate it. So, --testlist is rarely used, and I'm not sure what the value of it is. I think the best thing to do for now is to update the documentation to say something like "generates an script or text file containing a list of tests". Also, I discovered some errors which I will correct in the next patch
          Hide
          taher Taher Alkhateeb added a comment -

          Okay, this is what I consider the final patch, I fixed a bug and added proper documentation to --testlist.

          The target --testlist just generates a text file or an ant build script. You can test it right now with the syntax:

          java -jar ofbiz.jar --testlist file=testing.txt mode=text
          java -jar ofbiz.jar --testlist file=mybuild.xml mode=ant

          This makes this patch now as clean as possible. And I now understand exactly every command and how it works. I've cracked the code on this one

          Show
          taher Taher Alkhateeb added a comment - Okay, this is what I consider the final patch, I fixed a bug and added proper documentation to --testlist. The target --testlist just generates a text file or an ant build script. You can test it right now with the syntax: java -jar ofbiz.jar --testlist file=testing.txt mode=text java -jar ofbiz.jar --testlist file=mybuild.xml mode=ant This makes this patch now as clean as possible. And I now understand exactly every command and how it works. I've cracked the code on this one
          Hide
          pfm.smits Pierre Smits added a comment -

          Maybe we should consider moving the cobertura integration functionalities to the attic.

          Show
          pfm.smits Pierre Smits added a comment - Maybe we should consider moving the cobertura integration functionalities to the attic.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          After applying the last patch and an "ant clean-all load-demo" I tried

          1. java -jar ofbiz.jar --testlist
          2. java -jar ofbiz.jar --testlist file=testing.txt mode=text
          3. java -jar ofbiz.jar --testlist file=testing.txt mode=ant

          For 2 and 3 I get but the help. For 1 I get:

          C:\projectASF-Mars\ofbiz>java -jar ofbiz.jar --testlist
          Start.java using configuration file org/ofbiz/base/start/testlist.properties
          Set OFBIZ_HOME to - C:/projectASF-Mars/ofbiz
          Admin socket configured on - /127.0.0.1:10523
          2016-05-16 11:17:25,827 |main                 |ContainerLoader               |I| [Startup] Loading containers from C:/projectASF-Mars/ofbiz/framework/base/config/ofbiz-containers.xml for loaders [testlist]
          2016-05-16 11:17:26,001 |main                 |ContainerLoader               |I| Loading container: component-container
          2016-05-16 11:17:26,011 |main                 |ComponentContainer            |I| Auto-Loading component directory : [C:/projectASF-Mars/ofbiz/framework]
          [...]
          2016-05-16 11:17:26,923 |main                 |ComponentContainer            |I| All components loaded
          2016-05-16 11:17:26,923 |main                 |ContainerLoader               |I| Loaded container: component-container
          2016-05-16 11:17:26,923 |main                 |ContainerLoader               |I| Loading component's container: testtools-container
          2016-05-16 11:17:26,925 |main                 |ContainerLoader               |I| Loaded component's container: testtools-container
          2016-05-16 11:17:26,927 |main                 |ContainerLoader               |I| [Startup] Starting containers...
          2016-05-16 11:17:26,927 |main                 |ContainerLoader               |I| Starting container component-container
          2016-05-16 11:17:26,927 |main                 |ContainerLoader               |I| Started container component-container
          2016-05-16 11:17:26,927 |main                 |ContainerLoader               |I| Starting container testtools-container
          Exception in thread "main" java.lang.NullPointerException
                  at java.io.File.<init>(Unknown Source)
                  at org.ofbiz.testtools.TestListContainer.start(TestListContainer.java:111)
                  at org.ofbiz.base.container.ContainerLoader.start(ContainerLoader.java:237)
                  at org.ofbiz.base.start.Start.startStartLoaders(Start.java:440)
                  at org.ofbiz.base.start.Start.start(Start.java:166)
                  at org.ofbiz.base.start.Start.main(Start.java:100)
          2016-05-16 11:17:44,934 |Thread-0             |ContainerLoader               |I| Shutting down containers
          2016-05-16 11:17:44,934 |Thread-0             |ContainerLoader               |I| Stopping container testtools-container
          2016-05-16 11:17:44,934 |Thread-0             |ContainerLoader               |I| Stopped container testtools-container
          2016-05-16 11:17:44,934 |Thread-0             |ContainerLoader               |I| Stopping container component-container
          2016-05-16 11:17:44,934 |Thread-0             |ContainerLoader               |I| Stopped container component-container
          
          Show
          jacques.le.roux Jacques Le Roux added a comment - After applying the last patch and an "ant clean-all load-demo" I tried java -jar ofbiz.jar --testlist java -jar ofbiz.jar --testlist file=testing.txt mode=text java -jar ofbiz.jar --testlist file=testing.txt mode=ant For 2 and 3 I get but the help. For 1 I get: C:\projectASF-Mars\ofbiz>java -jar ofbiz.jar --testlist Start.java using configuration file org/ofbiz/base/start/testlist.properties Set OFBIZ_HOME to - C:/projectASF-Mars/ofbiz Admin socket configured on - /127.0.0.1:10523 2016-05-16 11:17:25,827 |main |ContainerLoader |I| [Startup] Loading containers from C:/projectASF-Mars/ofbiz/framework/base/config/ofbiz-containers.xml for loaders [testlist] 2016-05-16 11:17:26,001 |main |ContainerLoader |I| Loading container: component-container 2016-05-16 11:17:26,011 |main |ComponentContainer |I| Auto-Loading component directory : [C:/projectASF-Mars/ofbiz/framework] [...] 2016-05-16 11:17:26,923 |main |ComponentContainer |I| All components loaded 2016-05-16 11:17:26,923 |main |ContainerLoader |I| Loaded container: component-container 2016-05-16 11:17:26,923 |main |ContainerLoader |I| Loading component's container: testtools-container 2016-05-16 11:17:26,925 |main |ContainerLoader |I| Loaded component's container: testtools-container 2016-05-16 11:17:26,927 |main |ContainerLoader |I| [Startup] Starting containers... 2016-05-16 11:17:26,927 |main |ContainerLoader |I| Starting container component-container 2016-05-16 11:17:26,927 |main |ContainerLoader |I| Started container component-container 2016-05-16 11:17:26,927 |main |ContainerLoader |I| Starting container testtools-container Exception in thread "main" java.lang.NullPointerException at java.io.File.<init>(Unknown Source) at org.ofbiz.testtools.TestListContainer.start(TestListContainer.java:111) at org.ofbiz.base.container.ContainerLoader.start(ContainerLoader.java:237) at org.ofbiz.base.start.Start.startStartLoaders(Start.java:440) at org.ofbiz.base.start.Start.start(Start.java:166) at org.ofbiz.base.start.Start.main(Start.java:100) 2016-05-16 11:17:44,934 | Thread -0 |ContainerLoader |I| Shutting down containers 2016-05-16 11:17:44,934 | Thread -0 |ContainerLoader |I| Stopping container testtools-container 2016-05-16 11:17:44,934 | Thread -0 |ContainerLoader |I| Stopped container testtools-container 2016-05-16 11:17:44,934 | Thread -0 |ContainerLoader |I| Stopping container component-container 2016-05-16 11:17:44,934 | Thread -0 |ContainerLoader |I| Stopped container component-container
          Hide
          taher Taher Alkhateeb added a comment -

          in reply to jacques's comment, I made the arguments for --testlist mandatory

          Show
          taher Taher Alkhateeb added a comment - in reply to jacques's comment, I made the arguments for --testlist mandatory
          Hide
          taher Taher Alkhateeb added a comment -

          Fixed it in my latest patch. Please check.

          Show
          taher Taher Alkhateeb added a comment - Fixed it in my latest patch. Please check.
          Hide
          taher Taher Alkhateeb added a comment -

          I also want to note that there is more validation logic that needs to be added later, but I would like to commit first given the big size of this change already

          Show
          taher Taher Alkhateeb added a comment - I also want to note that there is more validation logic that needs to be added later, but I would like to commit first given the big size of this change already
          Hide
          taher Taher Alkhateeb added a comment -

          oops, please ignore, reattaching

          Show
          taher Taher Alkhateeb added a comment - oops, please ignore, reattaching
          Hide
          taher Taher Alkhateeb added a comment -

          latest attachment, minor error, I touched .project by mistake and now this is fixed

          Show
          taher Taher Alkhateeb added a comment - latest attachment, minor error, I touched .project by mistake and now this is fixed
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Please remove this part from your patch

          Index: framework/start/lib/commons-cli-1.3.1.jar
          ===================================================================
          Cannot display: file marked as a binary type.
          svn:mime-type = application/octet-stream
          Index: framework/start/lib/commons-cli-1.3.1.jar
          ===================================================================
          --- framework/start/lib/commons-cli-1.3.1.jar	(revision 0)
          +++ framework/start/lib/commons-cli-1.3.1.jar	(working copy)
          
          Property changes on: framework/start/lib/commons-cli-1.3.1.jar
          ___________________________________________________________________
          Added: svn:mime-type
          ## -0,0 +1 ##
          +application/octet-stream
          \ No newline at end of property
          

          Else you get this

          Buildfile: C:\projectASF-Mars\ofbiz\build.xml
          dir-init:
          copy-derby-props:
          ofbiz-init:
          build:
               [echo] [build] ========== Start Building (Compile) ==========
          build-dev:
          build-framework:
          init:
          prepare:
              [mkdir] Created dir: C:\projectASF-Mars\ofbiz\framework\start\build\classes
              [mkdir] Created dir: C:\projectASF-Mars\ofbiz\framework\start\build\lib
          classes:
            [javac18] Compiling 12 source files to C:\projectASF-Mars\ofbiz\framework\start\build\classes
            [javac18] error: error reading C:\projectASF-Mars\ofbiz\framework\start\lib\commons-cli-1.3.1.jar; error in opening zip file
            [javac18] 1 error
          
          BUILD FAILED
          C:\projectASF-Mars\ofbiz\build.xml:421: The following error occurred while executing this line:
          C:\projectASF-Mars\ofbiz\build.xml:438: The following error occurred while executing this line:
          C:\projectASF-Mars\ofbiz\macros.xml:39: The following error occurred while executing this line:
          C:\projectASF-Mars\ofbiz\framework\start\build.xml:53: Compile failed; see the compiler error output for details.
          
          Total time: 896 milliseconds
          

          Better let people put the lib there themselves.

          After putting back the lib by hand and doing and "ant clean build' I still get the same error with
          java -jar ofbiz.jar --testlist mode=ant
          or
          java -jar ofbiz.jar --testlist mode=text

          With
          java -jar ofbiz.jar --testlist file=runtime/test-list-build.xml
          I now get this list

          base:basetests
          entity:entitytests
          minilang:MinilangTests
          common:userlogintests
          common:performfindtests
          service:servicetests
          widget:widgettests
          party:partytests
          party:partycontactmechtests
          workeffort:workefforttests
          product:catalogtests
          product:facilitytests
          product:productcosttests
          product:groupordertests
          product:producttagtests
          manufacturing:productionruntests
          accounting:accountingtests
          accounting:paymenttests
          accounting:paymentappltests
          accounting:invoicetests
          accounting:fixedassettests
          order:finaccounttests
          order:ordertests
          order:custrequesttests
          order:quotetests
          order:shoppinglisttests
          order:shoppingcarttests
          marketing:marketingtests
          lucene:lucenetests
          assetmaint:assetmainttests
          scrum:scrumtests
          example:example-tests

          Not sure how this is useful...

          Show
          jacques.le.roux Jacques Le Roux added a comment - Please remove this part from your patch Index: framework/start/lib/commons-cli-1.3.1.jar =================================================================== Cannot display: file marked as a binary type. svn:mime-type = application/octet-stream Index: framework/start/lib/commons-cli-1.3.1.jar =================================================================== --- framework/start/lib/commons-cli-1.3.1.jar (revision 0) +++ framework/start/lib/commons-cli-1.3.1.jar (working copy) Property changes on: framework/start/lib/commons-cli-1.3.1.jar ___________________________________________________________________ Added: svn:mime-type ## -0,0 +1 ## +application/octet-stream \ No newline at end of property Else you get this Buildfile: C:\projectASF-Mars\ofbiz\build.xml dir-init: copy-derby-props: ofbiz-init: build: [echo] [build] ========== Start Building (Compile) ========== build-dev: build-framework: init: prepare: [mkdir] Created dir: C:\projectASF-Mars\ofbiz\framework\start\build\classes [mkdir] Created dir: C:\projectASF-Mars\ofbiz\framework\start\build\lib classes: [javac18] Compiling 12 source files to C:\projectASF-Mars\ofbiz\framework\start\build\classes [javac18] error: error reading C:\projectASF-Mars\ofbiz\framework\start\lib\commons-cli-1.3.1.jar; error in opening zip file [javac18] 1 error BUILD FAILED C:\projectASF-Mars\ofbiz\build.xml:421: The following error occurred while executing this line: C:\projectASF-Mars\ofbiz\build.xml:438: The following error occurred while executing this line: C:\projectASF-Mars\ofbiz\macros.xml:39: The following error occurred while executing this line: C:\projectASF-Mars\ofbiz\framework\start\build.xml:53: Compile failed; see the compiler error output for details. Total time: 896 milliseconds Better let people put the lib there themselves. After putting back the lib by hand and doing and "ant clean build' I still get the same error with java -jar ofbiz.jar --testlist mode=ant or java -jar ofbiz.jar --testlist mode=text With java -jar ofbiz.jar --testlist file=runtime/test-list-build.xml I now get this list base:basetests entity:entitytests minilang:MinilangTests common:userlogintests common:performfindtests service:servicetests widget:widgettests party:partytests party:partycontactmechtests workeffort:workefforttests product:catalogtests product:facilitytests product:productcosttests product:groupordertests product:producttagtests manufacturing:productionruntests accounting:accountingtests accounting:paymenttests accounting:paymentappltests accounting:invoicetests accounting:fixedassettests order:finaccounttests order:ordertests order:custrequesttests order:quotetests order:shoppinglisttests order:shoppingcarttests marketing:marketingtests lucene:lucenetests assetmaint:assetmainttests scrum:scrumtests example:example-tests Not sure how this is useful...
          Hide
          taher Taher Alkhateeb added a comment -

          Hi Jacques,

          I added full validation logic for --testlist and fixed the patch as you requested.

          I would like to ask you, however, to take it easy on the validation logic for now. There are many variations that could break the build and I want to handle them all for sure. But we are still in a much better shape than the past which was extremely brittle and if you change one thing everything breaks.

          Once I commit, I will move towards more robust validation rules for all the commands like for example portoffset should only accept integers, etc ...

          Waiting for your feedback and crossing fingers.

          Show
          taher Taher Alkhateeb added a comment - Hi Jacques, I added full validation logic for --testlist and fixed the patch as you requested. I would like to ask you, however, to take it easy on the validation logic for now. There are many variations that could break the build and I want to handle them all for sure. But we are still in a much better shape than the past which was extremely brittle and if you change one thing everything breaks. Once I commit, I will move towards more robust validation rules for all the commands like for example portoffset should only accept integers, etc ... Waiting for your feedback and crossing fingers.
          Hide
          jacques.le.roux Jacques Le Roux added a comment - - edited

          Sorry Taher,

          Still the same issue with --testlist mode=ant. Actually I believe it's not mandatory for now and could be fixed later.

          With the introduction of commons-cli-1.3.1.jar it's certainly a good idea to check parameters values. For instance, it's was not a big deal for me when I added the portoffset feature, but would be better to have it, I agree! BTW it's not even integers but roughly values < 2^16 (since port values vary and don't start from 0 the strict rule is hard to clearly establish and I'd not spent too much time on it )

          Show
          jacques.le.roux Jacques Le Roux added a comment - - edited Sorry Taher, Still the same issue with --testlist mode=ant . Actually I believe it's not mandatory for now and could be fixed later. With the introduction of commons-cli-1.3.1.jar it's certainly a good idea to check parameters values. For instance, it's was not a big deal for me when I added the portoffset feature, but would be better to have it, I agree! BTW it's not even integers but roughly values < 2^16 (since port values vary and don't start from 0 the strict rule is hard to clearly establish and I'd not spent too much time on it )
          Hide
          taher Taher Alkhateeb added a comment -

          Hi Jacques,

          When I run the following command:

          java -jar ofbiz.jar --testlist mode=ant

          I get the following error message -> Error: You must pass both file and mode arguments to --testlist

          Did you recompile?

          Show
          taher Taher Alkhateeb added a comment - Hi Jacques, When I run the following command: java -jar ofbiz.jar --testlist mode=ant I get the following error message -> Error: You must pass both file and mode arguments to --testlist Did you recompile?
          Hide
          taher Taher Alkhateeb added a comment -

          oh oh oh I think I know what you mean now. You are not inquiring about validation, but you just don't know what is the purpose of the generated text file.

          Ok, so the test list container generates either an ant file or a raw text file of all the tests as I documented. Why is a question left for another time, as nothing in build.xml is calling --testlist with mode=text. This is old history so it must've been used in the past or something.

          We can clean it up later, because it would be too much work now and this patch will be delayed if we start to delete this stuff.

          Show
          taher Taher Alkhateeb added a comment - oh oh oh I think I know what you mean now. You are not inquiring about validation, but you just don't know what is the purpose of the generated text file. Ok, so the test list container generates either an ant file or a raw text file of all the tests as I documented. Why is a question left for another time, as nothing in build.xml is calling --testlist with mode=text. This is old history so it must've been used in the past or something. We can clean it up later, because it would be too much work now and this patch will be delayed if we start to delete this stuff.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          OK, my bad I kept the old StartupCommand.java and StartupCommandUtil.java. I got it working using
          java -jar ofbiz.jar --testlist file=runtime/test-list-build.xml --testlist mode=ant
          It was surprising to me to have to put --testlist twice in the command line, but following the help made it clear (I guess a note about having the 2 --testlist options on the same line would help)

          I now understand your reluctance to remove it. I think all is clear now.

          Show
          jacques.le.roux Jacques Le Roux added a comment - OK, my bad I kept the old StartupCommand.java and StartupCommandUtil.java. I got it working using java -jar ofbiz.jar --testlist file=runtime/test-list-build.xml --testlist mode=ant It was surprising to me to have to put --testlist twice in the command line, but following the help made it clear (I guess a note about having the 2 --testlist options on the same line would help) I now understand your reluctance to remove it. I think all is clear now.
          Hide
          taher Taher Alkhateeb added a comment -

          Ding ding ding

          Ladies and Gentlemen boys and girls, we have a commit in r1744107. Crossing fingers and hoping for the best. I hope everyone likes this change.

          Thank you Jacques for the great help in testing.

          Show
          taher Taher Alkhateeb added a comment - Ding ding ding Ladies and Gentlemen boys and girls, we have a commit in r1744107. Crossing fingers and hoping for the best. I hope everyone likes this change. Thank you Jacques for the great help in testing.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Done with wiki update

          Show
          jacques.le.roux Jacques Le Roux added a comment - Done with wiki update
          Hide
          taher Taher Alkhateeb added a comment -

          Hi Jacques,

          It's still not fully updated. You need to remove the dash before the word readers. So you must change:
          java -jar ofbiz.jar --load-data -readers=seed,ext
          to
          java -jar ofbiz.jar --load-data readers=seed,ext

          I found that in two lines

          Show
          taher Taher Alkhateeb added a comment - Hi Jacques, It's still not fully updated. You need to remove the dash before the word readers. So you must change: java -jar ofbiz.jar --load-data -readers=seed,ext to java -jar ofbiz.jar --load-data readers=seed,ext I found that in two lines
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Thanks for the review, fixed!

          Show
          jacques.le.roux Jacques Le Roux added a comment - Thanks for the review, fixed!
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          At revision: 1744515, I Improved the visibility of the Start error messages

          Show
          jacques.le.roux Jacques Le Roux added a comment - At revision: 1744515, I Improved the visibility of the Start error messages
          Hide
          taher Taher Alkhateeb added a comment -

          Hi Jacques,

          This code does not look good, you shouldn't put formatting stuff in main. The same is true with StartupException. I suggest to create a prettyPrint function somewhere to take care of this if you want to box it with =====. Otherwise, again we are mixing different levels of abstraction and making it extremely dependent on the device output. This means that if we ever decide to change the formatting of the help screen, then I have to go and change it everywhere you made the modifications to make it look pretty.

          Also, I implemented commons-cli so that each command must be called multiple times if it takes multiple arguments. Look at the below examples
          Example: java -jar ofbiz.jar --load-data readers=seed --load-data delegator=default
          another example: java -jar ofbiz.jar --test component=base --test suiteName=whatever

          So my suggestion is to instead specify in the header or footer of the help menu that you must pass the commands multiple times and remove the ", ie 2 --testlist" which is confusing anyway.

          My intention of this refactoring is to make Start.java and espeially main very short and clean. Your modifications for one thing makes the line width 175 characters wide in addition to having something very unimportant like formatting right there in main where the programmer should not be worried with such details.

          Sorry for rambling for too long, but it is these small things over long time that made the code as fragile as it is right now. So please consider a pretty printer for your formatting and hide it away from main.

          Show
          taher Taher Alkhateeb added a comment - Hi Jacques, This code does not look good, you shouldn't put formatting stuff in main. The same is true with StartupException. I suggest to create a prettyPrint function somewhere to take care of this if you want to box it with =====. Otherwise, again we are mixing different levels of abstraction and making it extremely dependent on the device output. This means that if we ever decide to change the formatting of the help screen, then I have to go and change it everywhere you made the modifications to make it look pretty. Also, I implemented commons-cli so that each command must be called multiple times if it takes multiple arguments. Look at the below examples Example: java -jar ofbiz.jar --load-data readers=seed --load-data delegator=default another example: java -jar ofbiz.jar --test component=base --test suiteName=whatever So my suggestion is to instead specify in the header or footer of the help menu that you must pass the commands multiple times and remove the ", ie 2 --testlist" which is confusing anyway. My intention of this refactoring is to make Start.java and espeially main very short and clean. Your modifications for one thing makes the line width 175 characters wide in addition to having something very unimportant like formatting right there in main where the programmer should not be worried with such details. Sorry for rambling for too long, but it is these small things over long time that made the code as fragile as it is right now. So please consider a pretty printer for your formatting and hide it away from main.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          OK, I will simply revert and let you handling things, it's indeed only formatting anyway.

          BTW, I think I can't compare with what we had before because AFAIK we could only run all tests. But when I ran

          java -jar ofbiz.jar --test component=minilang --test

          I got this in stopping phase

          2016-05-19 09:22:51,685 |main                 |ContainerLoader               |I| Started container testtools-container
          2016-05-19 09:22:51,685 |main                 |ContainerLoader               |I| Shutting down containers
          2016-05-19 09:22:51,685 |main                 |ContainerLoader               |I| Stopping container testtools-container
          2016-05-19 09:22:51,685 |main                 |ContainerLoader               |I| Stopped container testtools-container
          2016-05-19 09:22:51,686 |main                 |ContainerLoader               |I| Stopping container catalina-container-test
          2016-05-19 09:22:51,844 |0.0.0.0-startStop-2  |ServiceContainer              |I| Removing from cache dispatcher: ebay
          2016-05-19 09:22:51,844 |0.0.0.0-startStop-2  |ServiceDispatcher             |I| De-Registering dispatcher: ebay
          2016-05-19 09:22:51,848 |0.0.0.0-startStop-2  |WebappClassLoaderBase         |W| The web application [ebay] appears to have started a thread named [OFBiz-JobPoller] but has failed to stop it. This is very lik
          ely to create a memory leak. Stack trace of thread:
           java.lang.Thread.sleep(Native Method)
           org.ofbiz.service.job.JobPoller$JobManagerPoller.run(JobPoller.java:214)
           java.lang.Thread.run(Unknown Source)
          2016-05-19 09:22:51,849 |0.0.0.0-startStop-2  |WebappClassLoaderBase         |W| The web application [ebay] appears to have started a thread named [RetryTimer] but has failed to stop it. This is very likely t
          o create a memory leak. Stack trace of thread:
           java.lang.Object.wait(Native Method)
           java.lang.Object.wait(Unknown Source)
           java.util.TimerThread.mainLoop(Unknown Source)
           java.util.TimerThread.run(Unknown Source)
          2016-05-19 09:22:51,849 |0.0.0.0-startStop-2  |WebappClassLoaderBase         |W| The web application [ebay] appears to have started a thread named [ForkJoinPool-1-worker-3] but has failed to stop it. This is
          very likely to create a memory leak. Stack trace of thread:
           sun.misc.Unsafe.park(Native Method)
           java.util.concurrent.ForkJoinPool.awaitWork(Unknown Source)
           java.util.concurrent.ForkJoinPool.runWorker(Unknown Source)
           java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source)
          2016-05-19 09:22:51,849 |0.0.0.0-startStop-2  |WebappClassLoaderBase         |W| The web application [ebay] appears to have started a thread named [ForkJoinPool-1-worker-5] but has failed to stop it. This is
          very likely to create a memory leak. Stack trace of thread:
           sun.misc.Unsafe.park(Native Method)
           java.util.concurrent.ForkJoinPool.awaitWork(Unknown Source)
           java.util.concurrent.ForkJoinPool.runWorker(Unknown Source)
           java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source)
          2016-05-19 09:22:51,849 |0.0.0.0-startStop-2  |WebappClassLoaderBase         |W| The web application [ebay] appears to have started a thread named [ForkJoinPool-1-worker-6] but has failed to stop it. This is
          very likely to create a memory leak. Stack trace of thread:
           sun.misc.Unsafe.park(Native Method)
           java.util.concurrent.ForkJoinPool.awaitWork(Unknown Source)
           java.util.concurrent.ForkJoinPool.runWorker(Unknown Source)
           java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source)
          2016-05-19 09:22:51,850 |0.0.0.0-startStop-2  |WebappClassLoaderBase         |W| The web application [ebay] appears to have started a thread named [ForkJoinPool-1-worker-7] but has failed to stop it. This is
          very likely to create a memory leak. Stack trace of thread:
           sun.misc.Unsafe.park(Native Method)
           java.util.concurrent.ForkJoinPool.awaitWork(Unknown Source)
           java.util.concurrent.ForkJoinPool.runWorker(Unknown Source)
           java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source)
          2016-05-19 09:22:51,856 |0.0.0.0-startStop-2  |ServiceContainer              |I| Removing from cache dispatcher: marketing
          2016-05-19 09:22:51,856 |0.0.0.0-startStop-2  |ServiceDispatcher             |I| De-Registering dispatcher: marketing
          2016-05-19 09:22:51,869 |0.0.0.0-startStop-2  |ServiceContainer              |I| Removing from cache dispatcher: oagismgr
          2016-05-19 09:22:51,869 |0.0.0.0-startStop-2  |ServiceDispatcher             |I| De-Registering dispatcher: oagismgr
          2016-05-19 09:22:51,875 |0.0.0.0-startStop-2  |ServiceContainer              |I| Removing from cache dispatcher: order
          2016-05-19 09:22:51,875 |0.0.0.0-startStop-2  |ServiceDispatcher             |I| De-Registering dispatcher: order
          2016-05-19 09:22:51,881 |0.0.0.0-startStop-2  |ServiceContainer              |I| Removing from cache dispatcher: googlecheckout
          2016-05-19 09:22:51,881 |0.0.0.0-startStop-2  |ServiceDispatcher             |I| De-Registering dispatcher: googlecheckout
          

          Not a big deal, and it seems not related with your changes, just wanted to let you know

          Show
          jacques.le.roux Jacques Le Roux added a comment - OK, I will simply revert and let you handling things, it's indeed only formatting anyway. BTW, I think I can't compare with what we had before because AFAIK we could only run all tests. But when I ran java -jar ofbiz.jar --test component=minilang --test I got this in stopping phase 2016-05-19 09:22:51,685 |main |ContainerLoader |I| Started container testtools-container 2016-05-19 09:22:51,685 |main |ContainerLoader |I| Shutting down containers 2016-05-19 09:22:51,685 |main |ContainerLoader |I| Stopping container testtools-container 2016-05-19 09:22:51,685 |main |ContainerLoader |I| Stopped container testtools-container 2016-05-19 09:22:51,686 |main |ContainerLoader |I| Stopping container catalina-container-test 2016-05-19 09:22:51,844 |0.0.0.0-startStop-2 |ServiceContainer |I| Removing from cache dispatcher: ebay 2016-05-19 09:22:51,844 |0.0.0.0-startStop-2 |ServiceDispatcher |I| De-Registering dispatcher: ebay 2016-05-19 09:22:51,848 |0.0.0.0-startStop-2 |WebappClassLoaderBase |W| The web application [ebay] appears to have started a thread named [OFBiz-JobPoller] but has failed to stop it. This is very lik ely to create a memory leak. Stack trace of thread: java.lang. Thread .sleep(Native Method) org.ofbiz.service.job.JobPoller$JobManagerPoller.run(JobPoller.java:214) java.lang. Thread .run(Unknown Source) 2016-05-19 09:22:51,849 |0.0.0.0-startStop-2 |WebappClassLoaderBase |W| The web application [ebay] appears to have started a thread named [RetryTimer] but has failed to stop it. This is very likely t o create a memory leak. Stack trace of thread: java.lang. Object .wait(Native Method) java.lang. Object .wait(Unknown Source) java.util.TimerThread.mainLoop(Unknown Source) java.util.TimerThread.run(Unknown Source) 2016-05-19 09:22:51,849 |0.0.0.0-startStop-2 |WebappClassLoaderBase |W| The web application [ebay] appears to have started a thread named [ForkJoinPool-1-worker-3] but has failed to stop it. This is very likely to create a memory leak. Stack trace of thread: sun.misc.Unsafe.park(Native Method) java.util.concurrent.ForkJoinPool.awaitWork(Unknown Source) java.util.concurrent.ForkJoinPool.runWorker(Unknown Source) java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source) 2016-05-19 09:22:51,849 |0.0.0.0-startStop-2 |WebappClassLoaderBase |W| The web application [ebay] appears to have started a thread named [ForkJoinPool-1-worker-5] but has failed to stop it. This is very likely to create a memory leak. Stack trace of thread: sun.misc.Unsafe.park(Native Method) java.util.concurrent.ForkJoinPool.awaitWork(Unknown Source) java.util.concurrent.ForkJoinPool.runWorker(Unknown Source) java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source) 2016-05-19 09:22:51,849 |0.0.0.0-startStop-2 |WebappClassLoaderBase |W| The web application [ebay] appears to have started a thread named [ForkJoinPool-1-worker-6] but has failed to stop it. This is very likely to create a memory leak. Stack trace of thread: sun.misc.Unsafe.park(Native Method) java.util.concurrent.ForkJoinPool.awaitWork(Unknown Source) java.util.concurrent.ForkJoinPool.runWorker(Unknown Source) java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source) 2016-05-19 09:22:51,850 |0.0.0.0-startStop-2 |WebappClassLoaderBase |W| The web application [ebay] appears to have started a thread named [ForkJoinPool-1-worker-7] but has failed to stop it. This is very likely to create a memory leak. Stack trace of thread: sun.misc.Unsafe.park(Native Method) java.util.concurrent.ForkJoinPool.awaitWork(Unknown Source) java.util.concurrent.ForkJoinPool.runWorker(Unknown Source) java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source) 2016-05-19 09:22:51,856 |0.0.0.0-startStop-2 |ServiceContainer |I| Removing from cache dispatcher: marketing 2016-05-19 09:22:51,856 |0.0.0.0-startStop-2 |ServiceDispatcher |I| De-Registering dispatcher: marketing 2016-05-19 09:22:51,869 |0.0.0.0-startStop-2 |ServiceContainer |I| Removing from cache dispatcher: oagismgr 2016-05-19 09:22:51,869 |0.0.0.0-startStop-2 |ServiceDispatcher |I| De-Registering dispatcher: oagismgr 2016-05-19 09:22:51,875 |0.0.0.0-startStop-2 |ServiceContainer |I| Removing from cache dispatcher: order 2016-05-19 09:22:51,875 |0.0.0.0-startStop-2 |ServiceDispatcher |I| De-Registering dispatcher: order 2016-05-19 09:22:51,881 |0.0.0.0-startStop-2 |ServiceContainer |I| Removing from cache dispatcher: googlecheckout 2016-05-19 09:22:51,881 |0.0.0.0-startStop-2 |ServiceDispatcher |I| De-Registering dispatcher: googlecheckout Not a big deal, and it seems not related with your changes, just wanted to let you know
          Hide
          taher Taher Alkhateeb added a comment -

          Hi Jacques, I guess need to further finetune the validation rules.

          The correct syntax is simple java -jar ofbiz.jar --test component=minilang

          You have an extra --test which is unneeded. I think I need to add too many validation rules

          Show
          taher Taher Alkhateeb added a comment - Hi Jacques, I guess need to further finetune the validation rules. The correct syntax is simple java -jar ofbiz.jar --test component=minilang You have an extra --test which is unneeded. I think I need to add too many validation rules
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Ah indeed, anyway same with

          java -jar ofbiz.jar --test component=minilang

          BTW when running

          java -jar ofbiz.jar test

          with your commit reverted (so old way)
          I get this at some point

          2016-05-19 09:46:43,169 |main                 |TestRunContainer              |I| [JUNIT] Pass: true | # Tests: 5 | # Failed: 0 # Errors: 0
          org.ofbiz.base.start.StartupException: Cannot start() org.ofbiz.testtools.TestRunContainer (Test run was unsuccessful)
                  at org.ofbiz.base.container.ContainerLoader.start(ContainerLoader.java:239)
                  at org.ofbiz.base.start.Start.startStartLoaders(Start.java:446)
                  at org.ofbiz.base.start.Start.start(Start.java:191)
                  at org.ofbiz.base.start.Start.main(Start.java:88)
          Caused by: org.ofbiz.base.container.ContainerException: Test run was unsuccessful
                  at org.ofbiz.testtools.TestRunContainer.start(TestRunContainer.java:193)
                  at org.ofbiz.base.container.ContainerLoader.start(ContainerLoader.java:237)
                  ... 3 more
          Exception in thread "main" org.ofbiz.base.start.StartupException: java.lang.Exception: Error during start. (Error during start.)
                  at org.ofbiz.base.start.Start.main(Start.java:91)
          Caused by: java.lang.Exception: Error during start.
                  at org.ofbiz.base.start.Start.start(Start.java:195)
                  at org.ofbiz.base.start.Start.main(Start.java:88)
          

          And then it's blocked. Anyway we don't care now... so just to say...

          Show
          jacques.le.roux Jacques Le Roux added a comment - Ah indeed, anyway same with java -jar ofbiz.jar --test component=minilang BTW when running java -jar ofbiz.jar test with your commit reverted (so old way) I get this at some point 2016-05-19 09:46:43,169 |main |TestRunContainer |I| [JUNIT] Pass: true | # Tests: 5 | # Failed: 0 # Errors: 0 org.ofbiz.base.start.StartupException: Cannot start() org.ofbiz.testtools.TestRunContainer (Test run was unsuccessful) at org.ofbiz.base.container.ContainerLoader.start(ContainerLoader.java:239) at org.ofbiz.base.start.Start.startStartLoaders(Start.java:446) at org.ofbiz.base.start.Start.start(Start.java:191) at org.ofbiz.base.start.Start.main(Start.java:88) Caused by: org.ofbiz.base.container.ContainerException: Test run was unsuccessful at org.ofbiz.testtools.TestRunContainer.start(TestRunContainer.java:193) at org.ofbiz.base.container.ContainerLoader.start(ContainerLoader.java:237) ... 3 more Exception in thread "main" org.ofbiz.base.start.StartupException: java.lang.Exception: Error during start. (Error during start.) at org.ofbiz.base.start.Start.main(Start.java:91) Caused by: java.lang.Exception: Error during start. at org.ofbiz.base.start.Start.start(Start.java:195) at org.ofbiz.base.start.Start.main(Start.java:88) And then it's blocked. Anyway we don't care now... so just to say...
          Hide
          taher Taher Alkhateeb added a comment -

          Hi Jacques,

          Thank you for reverting, I have implemented your work in r1744525. It's a simple prettyprinter in StartupCommandUtil that highlights your error message at the top.

          Show
          taher Taher Alkhateeb added a comment - Hi Jacques, Thank you for reverting, I have implemented your work in r1744525. It's a simple prettyprinter in StartupCommandUtil that highlights your error message at the top.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          And a Ctrl+C gave me that

          2016-05-19 09:46:43,169 |main                 |TestRunContainer              |I| [JUNIT] Pass: true | # Tests: 5 | # Failed: 0 # Errors: 0
          org.ofbiz.base.start.StartupException: Cannot start() org.ofbiz.testtools.TestRunContainer (Test run was unsuccessful)
                  at org.ofbiz.base.container.ContainerLoader.start(ContainerLoader.java:239)
                  at org.ofbiz.base.start.Start.startStartLoaders(Start.java:446)
                  at org.ofbiz.base.start.Start.start(Start.java:191)
                  at org.ofbiz.base.start.Start.main(Start.java:88)
          Caused by: org.ofbiz.base.container.ContainerException: Test run was unsuccessful
                  at org.ofbiz.testtools.TestRunContainer.start(TestRunContainer.java:193)
                  at org.ofbiz.base.container.ContainerLoader.start(ContainerLoader.java:237)
                  ... 3 more
          Exception in thread "main" org.ofbiz.base.start.StartupException: java.lang.Exception: Error during start. (Error during start.)
                  at org.ofbiz.base.start.Start.main(Start.java:91)
          Caused by: java.lang.Exception: Error during start.
                  at org.ofbiz.base.start.Start.start(Start.java:195)
                  at org.ofbiz.base.start.Start.main(Start.java:88)
          2016-05-19 09:48:28,669 |Thread-0             |ContainerLoader               |I| Shutting down containers
          2016-05-19 09:48:28,669 |Thread-0             |ContainerLoader               |I| Stopping container testtools-container
          2016-05-19 09:48:28,669 |Thread-0             |ContainerLoader               |I| Stopped container testtools-container
          2016-05-19 09:48:28,669 |Thread-0             |ContainerLoader               |I| Stopping container catalina-container-test
          2016-05-19 09:48:28,893 FATAL Unable to register shutdown hook because JVM is shutting down.
          2016-05-19 09:48:28,895 |0.0.0.0-startStop-2  |ServiceContainer              |I| Removing from cache dispatcher: ebay
          2016-05-19 09:48:28,895 |0.0.0.0-startStop-2  |ServiceDispatcher             |I| De-Registering dispatcher: ebay
          

          Note the

          FATAL Unable to register shutdown hook because JVM is shutting down.

          !
          Yours is far better!

          Show
          jacques.le.roux Jacques Le Roux added a comment - And a Ctrl+C gave me that 2016-05-19 09:46:43,169 |main |TestRunContainer |I| [JUNIT] Pass: true | # Tests: 5 | # Failed: 0 # Errors: 0 org.ofbiz.base.start.StartupException: Cannot start() org.ofbiz.testtools.TestRunContainer (Test run was unsuccessful) at org.ofbiz.base.container.ContainerLoader.start(ContainerLoader.java:239) at org.ofbiz.base.start.Start.startStartLoaders(Start.java:446) at org.ofbiz.base.start.Start.start(Start.java:191) at org.ofbiz.base.start.Start.main(Start.java:88) Caused by: org.ofbiz.base.container.ContainerException: Test run was unsuccessful at org.ofbiz.testtools.TestRunContainer.start(TestRunContainer.java:193) at org.ofbiz.base.container.ContainerLoader.start(ContainerLoader.java:237) ... 3 more Exception in thread "main" org.ofbiz.base.start.StartupException: java.lang.Exception: Error during start. (Error during start.) at org.ofbiz.base.start.Start.main(Start.java:91) Caused by: java.lang.Exception: Error during start. at org.ofbiz.base.start.Start.start(Start.java:195) at org.ofbiz.base.start.Start.main(Start.java:88) 2016-05-19 09:48:28,669 | Thread -0 |ContainerLoader |I| Shutting down containers 2016-05-19 09:48:28,669 | Thread -0 |ContainerLoader |I| Stopping container testtools-container 2016-05-19 09:48:28,669 | Thread -0 |ContainerLoader |I| Stopped container testtools-container 2016-05-19 09:48:28,669 | Thread -0 |ContainerLoader |I| Stopping container catalina-container-test 2016-05-19 09:48:28,893 FATAL Unable to register shutdown hook because JVM is shutting down. 2016-05-19 09:48:28,895 |0.0.0.0-startStop-2 |ServiceContainer |I| Removing from cache dispatcher: ebay 2016-05-19 09:48:28,895 |0.0.0.0-startStop-2 |ServiceDispatcher |I| De-Registering dispatcher: ebay Note the FATAL Unable to register shutdown hook because JVM is shutting down. ! Yours is far better!
          Hide
          taher Taher Alkhateeb added a comment -

          Hmmm, definitely worth investigating. It is my intention to attack all 16 containers once I'm done with refactoring the start component. I will definitely dig into this soon.

          Show
          taher Taher Alkhateeb added a comment - Hmmm, definitely worth investigating. It is my intention to attack all 16 containers once I'm done with refactoring the start component. I will definitely dig into this soon.
          Hide
          taher Taher Alkhateeb added a comment -

          Also another commit in r1744528 to clarify the usage in the footer.

          Show
          taher Taher Alkhateeb added a comment - Also another commit in r1744528 to clarify the usage in the footer.
          Hide
          jacques.le.roux Jacques Le Roux added a comment - - edited

          About

          java -jar ofbiz.jar --test component=minilang --test

          Actually I was abused by this line in help

          -t,--test <key=value>        Runs the selected test or all if none
                                        selected e.g.:
                                        --test component=base --test case=somecase
                                        --test component=base --test
                                        suitename=somesuite
          

          Could we not increase the width of the help?

          Show
          jacques.le.roux Jacques Le Roux added a comment - - edited About java -jar ofbiz.jar --test component=minilang --test Actually I was abused by this line in help -t,--test <key=value> Runs the selected test or all if none selected e.g.: --test component=base --test case =somecase --test component=base --test suitename=somesuite Could we not increase the width of the help?
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Indeed, this happens also with

          java -jar ofbiz.jar --test

          ie in your version. So it's not fixed by your changes (I see no reasons why it would, just saying )

          Show
          jacques.le.roux Jacques Le Roux added a comment - Indeed, this happens also with java -jar ofbiz.jar --test ie in your version. So it's not fixed by your changes (I see no reasons why it would, just saying )
          Hide
          taher Taher Alkhateeb added a comment -

          Yeah I see why you got confused by that.

          So there are two problems here:
          1- The second example breaks into two lines due to screen width
          2- It does not show like they are two separate examples, it looks like one single command

          Which one confuse you? or is it both? If so, I can make the screen a little wider for commons-cli and also add the word "OR between the two lines. What do you think?

          Show
          taher Taher Alkhateeb added a comment - Yeah I see why you got confused by that. So there are two problems here: 1- The second example breaks into two lines due to screen width 2- It does not show like they are two separate examples, it looks like one single command Which one confuse you? or is it both? If so, I can make the screen a little wider for commons-cli and also add the word "OR between the two lines. What do you think?
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Thanks!

          Show
          jacques.le.roux Jacques Le Roux added a comment - Thanks!
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          A wider screen would help readability, but there could still be cases, notably when maintained.
          So indeed a sign to say it continues on next line would be definitely better!

          BTW I don't think we need to parse too much, I mean eg no needs to test for a --test alone, etc.

          Show
          jacques.le.roux Jacques Le Roux added a comment - A wider screen would help readability, but there could still be cases, notably when maintained. So indeed a sign to say it continues on next line would be definitely better! BTW I don't think we need to parse too much, I mean eg no needs to test for a --test alone, etc.
          Hide
          taher Taher Alkhateeb added a comment -

          Ok, committed in r1744537. The screen width for commons-cli is now 80 characters instead of the default 74 (a little wider and nicer)

          I also fixed the --test documentation so that it is no longer confusing.

          Show
          taher Taher Alkhateeb added a comment - Ok, committed in r1744537. The screen width for commons-cli is now 80 characters instead of the default 74 (a little wider and nicer) I also fixed the --test documentation so that it is no longer confusing.
          Hide
          jacques.le.roux Jacques Le Roux added a comment - - edited

          Cool, could we not have also

          --testlist file=runtime/test-list-build.xml
          and
          --testlist mode=ant or --testlist mode=text
          

          I believe it's else still confusing, despite

          Also a command must be invoked separately for each argument e.g.
          java -jar ofbiz.jar --test component=somecomp --test case=somecase
          

          TL;DR always wins against RTFM!

          Show
          jacques.le.roux Jacques Le Roux added a comment - - edited Cool, could we not have also --testlist file=runtime/test-list-build.xml and --testlist mode=ant or --testlist mode=text I believe it's else still confusing, despite Also a command must be invoked separately for each argument e.g. java -jar ofbiz.jar --test component=somecomp --test case =somecase TL;DR always wins against RTFM!
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          This would need an extra Jira, but also some arguments... Why not improve it?

          Show
          jacques.le.roux Jacques Le Roux added a comment - This would need an extra Jira, but also some arguments... Why not improve it?
          Hide
          taher Taher Alkhateeb added a comment -

          done in r1744547.

          Show
          taher Taher Alkhateeb added a comment - done in r1744547.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Thks!

          Show
          jacques.le.roux Jacques Le Roux added a comment - Thks!
          Hide
          taher Taher Alkhateeb added a comment -

          This is a new big patch, and all tests run from my side. I made big changes and appreciate your feedback. What I've done is the following:

          • separate the AdminPortThread into a new class
          • delete CommonsDaemonStart
          • refactor everything so that all dependencies between classes are explicit through function calls (no hidden state or declarations)
          • move all the server processing logic (status, shutdown, start ...) to a new class called StartupControlPanel. All methods in this class are static with side effects only limited to the arguments passed to them.
          • add a few missing apache copyright headers
          • refactoring some names to make them clearer

          I know this is a big scary patch. But it makes reasoning about the code easier, and the next step is to fully refactor StartupControlPanel.start(...) to make it clean and efficient. But the main benefit of this patch is tangling all the dependency mess that used to exist.

          I look forward to hearing from you on this.

          Show
          taher Taher Alkhateeb added a comment - This is a new big patch, and all tests run from my side. I made big changes and appreciate your feedback. What I've done is the following: separate the AdminPortThread into a new class delete CommonsDaemonStart refactor everything so that all dependencies between classes are explicit through function calls (no hidden state or declarations) move all the server processing logic (status, shutdown, start ...) to a new class called StartupControlPanel. All methods in this class are static with side effects only limited to the arguments passed to them. add a few missing apache copyright headers refactoring some names to make them clearer I know this is a big scary patch. But it makes reasoning about the code easier, and the next step is to fully refactor StartupControlPanel.start(...) to make it clean and efficient. But the main benefit of this patch is tangling all the dependency mess that used to exist. I look forward to hearing from you on this.
          Hide
          taher Taher Alkhateeb added a comment -

          This is the latest version of the above mentioned patch. This adds the following in addition to everything in the previous patch

          • refactor Config.java and make it much cleaner and simpler by breaking things down into private methods
          • renaming and minor refactoring everywhere
          • make most package classes non public (package protected) to offer more isolation.

          I think if we apply this patch, then it is the last "major" refactoring of the start component.

          Show
          taher Taher Alkhateeb added a comment - This is the latest version of the above mentioned patch. This adds the following in addition to everything in the previous patch refactor Config.java and make it much cleaner and simpler by breaking things down into private methods renaming and minor refactoring everywhere make most package classes non public (package protected) to offer more isolation. I think if we apply this patch, then it is the last "major" refactoring of the start component.
          Hide
          taher Taher Alkhateeb added a comment -

          Note:

          For testing purposes and for best results, we need to test on multiple threads e.g.:

          • start the server
          • stop it in the middle of the startup starting
          • pretty much all try out all the commands in java -jar ofbiz.jar
          • start the server multiple times
          • of course run the tests with ./ant clean-all load-demo run-tests
          • run it for a while and make sure everything is stable
          Show
          taher Taher Alkhateeb added a comment - Note: For testing purposes and for best results, we need to test on multiple threads e.g.: start the server stop it in the middle of the startup starting pretty much all try out all the commands in java -jar ofbiz.jar start the server multiple times of course run the tests with ./ant clean-all load-demo run-tests run it for a while and make sure everything is stable
          Hide
          taher Taher Alkhateeb added a comment -

          I just realized that the loaders and adminPortThread in Start.java are unnecessary and redundant. So I removed them in this patch to be directly created in StartupControlPanel.start. This also makes main() much more elegant

          Show
          taher Taher Alkhateeb added a comment - I just realized that the loaders and adminPortThread in Start.java are unnecessary and redundant. So I removed them in this patch to be directly created in StartupControlPanel.start. This also makes main() much more elegant
          Hide
          taher Taher Alkhateeb added a comment -

          one more patch added, this adds further fine tuning by:

          • cleaning up Config.java some more
          • declaring some classes final
          Show
          taher Taher Alkhateeb added a comment - one more patch added, this adds further fine tuning by: cleaning up Config.java some more declaring some classes final
          Hide
          jacopoc Jacopo Cappellato added a comment -

          I have tested your last patch on a MacBook Pro running OS X 10.11.4 with Java 1.8.0_45 and it worked fine: all tests successful and I didn't notice any issues at startup or using the applications so far.

          Show
          jacopoc Jacopo Cappellato added a comment - I have tested your last patch on a MacBook Pro running OS X 10.11.4 with Java 1.8.0_45 and it worked fine: all tests successful and I didn't notice any issues at startup or using the applications so far.
          Hide
          gil portenseigne Gil Portenseigne added a comment -

          I just did the same on ubuntu with java version "1.8.0_72". No issue found !

          Show
          gil portenseigne Gil Portenseigne added a comment - I just did the same on ubuntu with java version "1.8.0_72". No issue found !
          Hide
          taher Taher Alkhateeb added a comment -

          Thank you Jacopo and Gil for your valuable feedback.

          I have committed the code in r1745351 with two minor adjustments:

          • Config.portOffset is int instead of the boxed Integer
          • changed StartupControlPanel.shutdownServer() to be private instead of default

          Tests all pass at my side as well on 1.8.0_91 in linux-64 both Mint and Arch and I see no anomalies thankfully.

          Show
          taher Taher Alkhateeb added a comment - Thank you Jacopo and Gil for your valuable feedback. I have committed the code in r1745351 with two minor adjustments: Config.portOffset is int instead of the boxed Integer changed StartupControlPanel.shutdownServer() to be private instead of default Tests all pass at my side as well on 1.8.0_91 in linux-64 both Mint and Arch and I see no anomalies thankfully.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Maybe not a big deal, but at the end of running

          java -jar ofbiz.jar --test component=minilang --test suitename=MinilangTests

          I got

          2016-05-25 10:33:55,357 |0.0.0.0-startStop-1  |ServiceDispatcher             |I| De-Registering dispatcher: scrum
          2016-05-25 10:33:56,082 |dExecutor-6-thread-1 |CoreContainer                 |E| Error creating core [solrdefault]: This CoreContainer has been close
          java.lang.IllegalStateException: This CoreContainer has been close
                  at org.apache.solr.core.CoreContainer.registerCore(CoreContainer.java:658) ~[solr-core-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:15]
                  at org.apache.solr.core.CoreContainer.create(CoreContainer.java:731) [solr-core-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:15]
                  at org.apache.solr.core.CoreContainer$1.call(CoreContainer.java:443) [solr-core-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:15]
                  at org.apache.solr.core.CoreContainer$1.call(CoreContainer.java:434) [solr-core-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:15]
                  at java.util.concurrent.FutureTask.run(Unknown Source) [?:1.8.0_91]
                  at org.apache.solr.common.util.ExecutorUtil$MDCAwareThreadPoolExecutor$1.run(ExecutorUtil.java:210) [solr-solrj-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:17]
                  at java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source) [?:1.8.0_91]
                  at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source) [?:1.8.0_91]
                  at java.lang.Thread.run(Unknown Source) [?:1.8.0_91]
          2016-05-25 10:33:56,082 |kExecutor-2-thread-1 |CoreContainer                 |E| Error waiting for SolrCore to be created
          java.util.concurrent.ExecutionException: org.apache.solr.common.SolrException: Unable to create core [solrdefault]
                  at java.util.concurrent.FutureTask.report(Unknown Source) ~[?:1.8.0_91]
                  at java.util.concurrent.FutureTask.get(Unknown Source) ~[?:1.8.0_91]
                  at org.apache.solr.core.CoreContainer$2.run(CoreContainer.java:472) [solr-core-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:15]
                  at java.util.concurrent.Executors$RunnableAdapter.call(Unknown Source) [?:1.8.0_91]
                  at java.util.concurrent.FutureTask.run(Unknown Source) [?:1.8.0_91]
                  at org.apache.solr.common.util.ExecutorUtil$MDCAwareThreadPoolExecutor$1.run(ExecutorUtil.java:210) [solr-solrj-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:17]
                  at java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source) [?:1.8.0_91]
                  at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source) [?:1.8.0_91]
                  at java.lang.Thread.run(Unknown Source) [?:1.8.0_91]
          Caused by: org.apache.solr.common.SolrException: Unable to create core [solrdefault]
                  at org.apache.solr.core.CoreContainer.create(CoreContainer.java:737) ~[solr-core-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:15]
                  at org.apache.solr.core.CoreContainer$1.call(CoreContainer.java:443) ~[solr-core-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:15]
                  at org.apache.solr.core.CoreContainer$1.call(CoreContainer.java:434) ~[solr-core-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:15]
                  ... 5 more
          Caused by: java.lang.IllegalStateException: This CoreContainer has been close
                  at org.apache.solr.core.CoreContainer.registerCore(CoreContainer.java:658) ~[solr-core-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:15]
                  at org.apache.solr.core.CoreContainer.create(CoreContainer.java:731) ~[solr-core-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:15]
                  at org.apache.solr.core.CoreContainer$1.call(CoreContainer.java:443) ~[solr-core-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:15]
                  at org.apache.solr.core.CoreContainer$1.call(CoreContainer.java:434) ~[solr-core-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:15]
                  ... 5 more
          2016-05-25 10:33:56,085 |0.0.0.0-startStop-1  |ServiceContainer              |I| Removing from cache dispatcher: solr
          2016-05-25 10:33:56,085 |0.0.0.0-startStop-1  |ServiceDispatcher             |I| De-Registering dispatcher: solr
          2016-05-25 10:33:56,089 |0.0.0.0-startStop-1  |ServiceContainer              |I| Removing from cache dispatcher: projectmgr
          2016-05-25 10:33:56,089 |0.0.0.0-startStop-1  |ServiceDispatcher             |I| De-Registering dispatcher: projectmgr
          

          I expect to review the whole commit soon...

          Show
          jacques.le.roux Jacques Le Roux added a comment - Maybe not a big deal, but at the end of running java -jar ofbiz.jar --test component=minilang --test suitename=MinilangTests I got 2016-05-25 10:33:55,357 |0.0.0.0-startStop-1 |ServiceDispatcher |I| De-Registering dispatcher: scrum 2016-05-25 10:33:56,082 |dExecutor-6-thread-1 |CoreContainer |E| Error creating core [solrdefault]: This CoreContainer has been close java.lang.IllegalStateException: This CoreContainer has been close at org.apache.solr.core.CoreContainer.registerCore(CoreContainer.java:658) ~[solr-core-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:15] at org.apache.solr.core.CoreContainer.create(CoreContainer.java:731) [solr-core-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:15] at org.apache.solr.core.CoreContainer$1.call(CoreContainer.java:443) [solr-core-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:15] at org.apache.solr.core.CoreContainer$1.call(CoreContainer.java:434) [solr-core-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:15] at java.util.concurrent.FutureTask.run(Unknown Source) [?:1.8.0_91] at org.apache.solr.common.util.ExecutorUtil$MDCAwareThreadPoolExecutor$1.run(ExecutorUtil.java:210) [solr-solrj-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:17] at java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source) [?:1.8.0_91] at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source) [?:1.8.0_91] at java.lang. Thread .run(Unknown Source) [?:1.8.0_91] 2016-05-25 10:33:56,082 |kExecutor-2-thread-1 |CoreContainer |E| Error waiting for SolrCore to be created java.util.concurrent.ExecutionException: org.apache.solr.common.SolrException: Unable to create core [solrdefault] at java.util.concurrent.FutureTask.report(Unknown Source) ~[?:1.8.0_91] at java.util.concurrent.FutureTask.get(Unknown Source) ~[?:1.8.0_91] at org.apache.solr.core.CoreContainer$2.run(CoreContainer.java:472) [solr-core-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:15] at java.util.concurrent.Executors$RunnableAdapter.call(Unknown Source) [?:1.8.0_91] at java.util.concurrent.FutureTask.run(Unknown Source) [?:1.8.0_91] at org.apache.solr.common.util.ExecutorUtil$MDCAwareThreadPoolExecutor$1.run(ExecutorUtil.java:210) [solr-solrj-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:17] at java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source) [?:1.8.0_91] at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source) [?:1.8.0_91] at java.lang. Thread .run(Unknown Source) [?:1.8.0_91] Caused by: org.apache.solr.common.SolrException: Unable to create core [solrdefault] at org.apache.solr.core.CoreContainer.create(CoreContainer.java:737) ~[solr-core-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:15] at org.apache.solr.core.CoreContainer$1.call(CoreContainer.java:443) ~[solr-core-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:15] at org.apache.solr.core.CoreContainer$1.call(CoreContainer.java:434) ~[solr-core-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:15] ... 5 more Caused by: java.lang.IllegalStateException: This CoreContainer has been close at org.apache.solr.core.CoreContainer.registerCore(CoreContainer.java:658) ~[solr-core-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:15] at org.apache.solr.core.CoreContainer.create(CoreContainer.java:731) ~[solr-core-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:15] at org.apache.solr.core.CoreContainer$1.call(CoreContainer.java:443) ~[solr-core-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:15] at org.apache.solr.core.CoreContainer$1.call(CoreContainer.java:434) ~[solr-core-5.3.1.jar:5.3.1 1703449 - noble - 2015-09-17 01:48:15] ... 5 more 2016-05-25 10:33:56,085 |0.0.0.0-startStop-1 |ServiceContainer |I| Removing from cache dispatcher: solr 2016-05-25 10:33:56,085 |0.0.0.0-startStop-1 |ServiceDispatcher |I| De-Registering dispatcher: solr 2016-05-25 10:33:56,089 |0.0.0.0-startStop-1 |ServiceContainer |I| Removing from cache dispatcher: projectmgr 2016-05-25 10:33:56,089 |0.0.0.0-startStop-1 |ServiceDispatcher |I| De-Registering dispatcher: projectmgr I expect to review the whole commit soon...
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          To help more about the point above here are attached the logs (error.log and ofbiz.log) after a

          ant clean-logs && java -jar ofbiz.jar --test component=minilang --test suitename=MinilangTests

          Show
          jacques.le.roux Jacques Le Roux added a comment - To help more about the point above here are attached the logs (error.log and ofbiz.log) after a ant clean-logs && java -jar ofbiz.jar --test component=minilang --test suitename=MinilangTests
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          This of course in a clean up to date WC

          Show
          jacques.le.roux Jacques Le Roux added a comment - This of course in a clean up to date WC
          Hide
          taher Taher Alkhateeb added a comment -

          Hi jacques,

          Is this behavior non existent pre patch. it seems only related to the solr component!

          Show
          taher Taher Alkhateeb added a comment - Hi jacques, Is this behavior non existent pre patch. it seems only related to the solr component!
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Forget this report, it's only in a WC with WIP for OFBIZ-6849

          Show
          jacques.le.roux Jacques Le Roux added a comment - Forget this report, it's only in a WC with WIP for OFBIZ-6849
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          This (kinda) pre-existed to your changes, in a WC at r1744041 running

          java -jar ofbiz.jar test

          I get also

          2016-05-25 14:04:08,996 |0.0.0.0-startStop-2  |ServiceDispatcher             |I| De-Registering dispatcher: ebay
          2016-05-25 14:04:09,001 |0.0.0.0-startStop-2  |WebappClassLoaderBase         |W| The web application [ebay] appears to have started a thread named [OFBiz-JobPoller] but has failed to stop it. This is very lik
          ely to create a memory leak. Stack trace of thread:
           java.lang.Thread.sleep(Native Method)
           org.ofbiz.service.job.JobPoller$JobManagerPoller.run(JobPoller.java:214)
           java.lang.Thread.run(Unknown Source)
          2016-05-25 14:04:09,002 |0.0.0.0-startStop-2  |WebappClassLoaderBase         |W| The web application [ebay] appears to have started a thread named [RetryTimer] but has failed to stop it. This is very likely t
          o create a memory leak. Stack trace of thread:
           java.lang.Object.wait(Native Method)
           java.lang.Object.wait(Unknown Source)
           java.util.TimerThread.mainLoop(Unknown Source)
           java.util.TimerThread.run(Unknown Source)
          2016-05-25 14:04:09,022 |0.0.0.0-startStop-2  |ServiceContainer              |I| Removing from cache dispatcher: marketing
          

          Of course if it would be cleaner it would not hurt, but this is only during tests and with now 16GB on most machines I guess it's does not have a huge impact... So later...

          Show
          jacques.le.roux Jacques Le Roux added a comment - This (kinda) pre-existed to your changes, in a WC at r1744041 running java -jar ofbiz.jar test I get also 2016-05-25 14:04:08,996 |0.0.0.0-startStop-2 |ServiceDispatcher |I| De-Registering dispatcher: ebay 2016-05-25 14:04:09,001 |0.0.0.0-startStop-2 |WebappClassLoaderBase |W| The web application [ebay] appears to have started a thread named [OFBiz-JobPoller] but has failed to stop it. This is very lik ely to create a memory leak. Stack trace of thread: java.lang. Thread .sleep(Native Method) org.ofbiz.service.job.JobPoller$JobManagerPoller.run(JobPoller.java:214) java.lang. Thread .run(Unknown Source) 2016-05-25 14:04:09,002 |0.0.0.0-startStop-2 |WebappClassLoaderBase |W| The web application [ebay] appears to have started a thread named [RetryTimer] but has failed to stop it. This is very likely t o create a memory leak. Stack trace of thread: java.lang. Object .wait(Native Method) java.lang. Object .wait(Unknown Source) java.util.TimerThread.mainLoop(Unknown Source) java.util.TimerThread.run(Unknown Source) 2016-05-25 14:04:09,022 |0.0.0.0-startStop-2 |ServiceContainer |I| Removing from cache dispatcher: marketing Of course if it would be cleaner it would not hurt, but this is only during tests and with now 16GB on most machines I guess it's does not have a huge impact... So later...
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Mmm wait, I confused, it's not the same kind of error, I lost myself working in 3 different WCs :/

          Also I must say that I can really compare because "--test component=minilang --test suitename=MinilangTests" did not exist before...

          So no I don't reproduce the same with a WC w/o your changes, only the possible memory leaks. But I have the same with "ant run-tests" in an up to date WC.

          I also noticed that there are others differences with "-jar ofbiz.jar --test" and "ant run-tests"
          Notably the tests passes with a WC with changes for OFBIZ-6849 using "-jar ofbiz.jar --test" but not with "ant run-tests", we are both in troubles

          Courage, I continue...

          Show
          jacques.le.roux Jacques Le Roux added a comment - Mmm wait, I confused, it's not the same kind of error, I lost myself working in 3 different WCs :/ Also I must say that I can really compare because "--test component=minilang --test suitename=MinilangTests" did not exist before... So no I don't reproduce the same with a WC w/o your changes, only the possible memory leaks. But I have the same with "ant run-tests" in an up to date WC. I also noticed that there are others differences with "-jar ofbiz.jar --test" and "ant run-tests" Notably the tests passes with a WC with changes for OFBIZ-6849 using "-jar ofbiz.jar --test" but not with "ant run-tests", we are both in troubles Courage, I continue...
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          OK I was wrong, at least in an up to date working copy with my changes for OFBIZ-6849 "ant run-tests" and "java -jar ofbiz.jar --test" behave the same (I was surprised by that)

          Show
          jacques.le.roux Jacques Le Roux added a comment - OK I was wrong, at least in an up to date working copy with my changes for OFBIZ-6849 "ant run-tests" and "java -jar ofbiz.jar --test" behave the same (I was surprised by that)
          Hide
          taher Taher Alkhateeb added a comment -

          I'm a bit lost now.

          First, the test case above "did" exist. It just had a different syntax which is:
          java -jar ofbiz.jar test -component=minilang -suitename=MinilangTests (before any of my work)

          Second, you need to go to an old version, run the tests, and see if the logs spit out any errors. I'm not sure where to debug if I can't determine for sure that these errors are related to my work or not.

          Either way, I suspect that it is not related to the patch because the system is complaining from specialpurpose components, which are far away from anything related to StartupLoaders (except for POS which is not showing in the logs)

          Is it possible to isolate my work and yours away from a comparison test?

          Show
          taher Taher Alkhateeb added a comment - I'm a bit lost now. First, the test case above "did" exist. It just had a different syntax which is: java -jar ofbiz.jar test -component=minilang -suitename=MinilangTests (before any of my work) Second, you need to go to an old version, run the tests, and see if the logs spit out any errors. I'm not sure where to debug if I can't determine for sure that these errors are related to my work or not. Either way, I suspect that it is not related to the patch because the system is complaining from specialpurpose components, which are far away from anything related to StartupLoaders (except for POS which is not showing in the logs) Is it possible to isolate my work and yours away from a comparison test?
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Ha I never used this syntax, only the ant target.
          OK running

          java -jar ofbiz.jar test -component=minilang -suitename=MinilangTests

          with a clean WC at r1744041 (the commit done the 2016-05-16 just before your 1st on start)
          I get

          2016-05-25 17:38:24,900 |main                 |ContainerLoader               |I| Stopping container catalina-container-test
          2016-05-25 17:38:25,019 |0.0.0.0-startStop-2  |ServiceContainer              |I| Removing from cache dispatcher: ebay
          2016-05-25 17:38:25,020 |0.0.0.0-startStop-2  |ServiceDispatcher             |I| De-Registering dispatcher: ebay
          2016-05-25 17:38:25,026 |0.0.0.0-startStop-2  |WebappClassLoaderBase         |W| The web application [ebay] appears to have started a thread named [OFBiz-JobPoller] but has failed to stop it. This is very lik
          ely to create a memory leak. Stack trace of thread:
           java.lang.Thread.sleep(Native Method)
           org.ofbiz.service.job.JobPoller$JobManagerPoller.run(JobPoller.java:214)
           java.lang.Thread.run(Unknown Source)
          2016-05-25 17:38:25,026 |0.0.0.0-startStop-2  |WebappClassLoaderBase         |W| The web application [ebay] appears to have started a thread named [RetryTimer] but has failed to stop it. This is very likely t
          o create a memory leak. Stack trace of thread:
           java.lang.Object.wait(Native Method)
           java.lang.Object.wait(Unknown Source)
           java.util.TimerThread.mainLoop(Unknown Source)
           java.util.TimerThread.run(Unknown Source)
          2016-05-25 17:38:25,026 |0.0.0.0-startStop-2  |WebappClassLoaderBase         |W| The web application [ebay] appears to have started a thread named [ForkJoinPool-1-worker-2] but has failed to stop it. This is
          very likely to create a memory leak. Stack trace of thread:
           sun.misc.Unsafe.park(Native Method)
           java.util.concurrent.ForkJoinPool.awaitWork(Unknown Source)
           java.util.concurrent.ForkJoinPool.runWorker(Unknown Source)
           java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source)
          2016-05-25 17:38:25,026 |0.0.0.0-startStop-2  |WebappClassLoaderBase         |W| The web application [ebay] appears to have started a thread named [ForkJoinPool-1-worker-3] but has failed to stop it. This is
          very likely to create a memory leak. Stack trace of thread:
           sun.misc.Unsafe.park(Native Method)
           java.util.concurrent.ForkJoinPool.awaitWork(Unknown Source)
           java.util.concurrent.ForkJoinPool.runWorker(Unknown Source)
           java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source)
          2016-05-25 17:38:25,026 |0.0.0.0-startStop-2  |WebappClassLoaderBase         |W| The web application [ebay] appears to have started a thread named [ForkJoinPool-1-worker-5] but has failed to stop it. This is
          very likely to create a memory leak. Stack trace of thread:
           sun.misc.Unsafe.park(Native Method)
           java.util.concurrent.ForkJoinPool.awaitWork(Unknown Source)
           java.util.concurrent.ForkJoinPool.runWorker(Unknown Source)
           java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source)
          2016-05-25 17:38:25,027 |0.0.0.0-startStop-2  |WebappClassLoaderBase         |W| The web application [ebay] appears to have started a thread named [ForkJoinPool-1-worker-6] but has failed to stop it. This is
          very likely to create a memory leak. Stack trace of thread:
           sun.misc.Unsafe.park(Native Method)
           java.util.concurrent.ForkJoinPool.awaitWork(Unknown Source)
           java.util.concurrent.ForkJoinPool.runWorker(Unknown Source)
           java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source)
          2016-05-25 17:38:25,027 |0.0.0.0-startStop-2  |WebappClassLoaderBase         |W| The web application [ebay] appears to have started a thread named [ForkJoinPool-1-worker-0] but has failed to stop it. This is
          very likely to create a memory leak. Stack trace of thread:
           sun.misc.Unsafe.park(Native Method)
           java.util.concurrent.ForkJoinPool.awaitWork(Unknown Source)
           java.util.concurrent.ForkJoinPool.runWorker(Unknown Source)
           java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source)
          2016-05-25 17:38:25,031 |0.0.0.0-startStop-2  |ServiceContainer              |I| Removing from cache dispatcher: marketing
          2016-05-25 17:38:25,031 |0.0.0.0-startStop-2  |ServiceDispatcher             |I| De-Registering dispatcher: marketing
          2016-05-25 17:38:25,046 |0.0.0.0-startStop-2  |ServiceContainer              |I| Removing from cache dispatcher: oagismgr
          

          So this one is surely not related to your changes.

          For the solr issue it's unclear but it does not appear with the test above. And the last change in solr component are the 2016-05-07 (r1742714).

          This said I don't think those should stop us. They need to be fixed, but not necessarily now... It's always a matter of priorities...

          Show
          jacques.le.roux Jacques Le Roux added a comment - Ha I never used this syntax, only the ant target. OK running java -jar ofbiz.jar test -component=minilang -suitename=MinilangTests with a clean WC at r1744041 (the commit done the 2016-05-16 just before your 1st on start) I get 2016-05-25 17:38:24,900 |main |ContainerLoader |I| Stopping container catalina-container-test 2016-05-25 17:38:25,019 |0.0.0.0-startStop-2 |ServiceContainer |I| Removing from cache dispatcher: ebay 2016-05-25 17:38:25,020 |0.0.0.0-startStop-2 |ServiceDispatcher |I| De-Registering dispatcher: ebay 2016-05-25 17:38:25,026 |0.0.0.0-startStop-2 |WebappClassLoaderBase |W| The web application [ebay] appears to have started a thread named [OFBiz-JobPoller] but has failed to stop it. This is very lik ely to create a memory leak. Stack trace of thread: java.lang. Thread .sleep(Native Method) org.ofbiz.service.job.JobPoller$JobManagerPoller.run(JobPoller.java:214) java.lang. Thread .run(Unknown Source) 2016-05-25 17:38:25,026 |0.0.0.0-startStop-2 |WebappClassLoaderBase |W| The web application [ebay] appears to have started a thread named [RetryTimer] but has failed to stop it. This is very likely t o create a memory leak. Stack trace of thread: java.lang. Object .wait(Native Method) java.lang. Object .wait(Unknown Source) java.util.TimerThread.mainLoop(Unknown Source) java.util.TimerThread.run(Unknown Source) 2016-05-25 17:38:25,026 |0.0.0.0-startStop-2 |WebappClassLoaderBase |W| The web application [ebay] appears to have started a thread named [ForkJoinPool-1-worker-2] but has failed to stop it. This is very likely to create a memory leak. Stack trace of thread: sun.misc.Unsafe.park(Native Method) java.util.concurrent.ForkJoinPool.awaitWork(Unknown Source) java.util.concurrent.ForkJoinPool.runWorker(Unknown Source) java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source) 2016-05-25 17:38:25,026 |0.0.0.0-startStop-2 |WebappClassLoaderBase |W| The web application [ebay] appears to have started a thread named [ForkJoinPool-1-worker-3] but has failed to stop it. This is very likely to create a memory leak. Stack trace of thread: sun.misc.Unsafe.park(Native Method) java.util.concurrent.ForkJoinPool.awaitWork(Unknown Source) java.util.concurrent.ForkJoinPool.runWorker(Unknown Source) java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source) 2016-05-25 17:38:25,026 |0.0.0.0-startStop-2 |WebappClassLoaderBase |W| The web application [ebay] appears to have started a thread named [ForkJoinPool-1-worker-5] but has failed to stop it. This is very likely to create a memory leak. Stack trace of thread: sun.misc.Unsafe.park(Native Method) java.util.concurrent.ForkJoinPool.awaitWork(Unknown Source) java.util.concurrent.ForkJoinPool.runWorker(Unknown Source) java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source) 2016-05-25 17:38:25,027 |0.0.0.0-startStop-2 |WebappClassLoaderBase |W| The web application [ebay] appears to have started a thread named [ForkJoinPool-1-worker-6] but has failed to stop it. This is very likely to create a memory leak. Stack trace of thread: sun.misc.Unsafe.park(Native Method) java.util.concurrent.ForkJoinPool.awaitWork(Unknown Source) java.util.concurrent.ForkJoinPool.runWorker(Unknown Source) java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source) 2016-05-25 17:38:25,027 |0.0.0.0-startStop-2 |WebappClassLoaderBase |W| The web application [ebay] appears to have started a thread named [ForkJoinPool-1-worker-0] but has failed to stop it. This is very likely to create a memory leak. Stack trace of thread: sun.misc.Unsafe.park(Native Method) java.util.concurrent.ForkJoinPool.awaitWork(Unknown Source) java.util.concurrent.ForkJoinPool.runWorker(Unknown Source) java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source) 2016-05-25 17:38:25,031 |0.0.0.0-startStop-2 |ServiceContainer |I| Removing from cache dispatcher: marketing 2016-05-25 17:38:25,031 |0.0.0.0-startStop-2 |ServiceDispatcher |I| De-Registering dispatcher: marketing 2016-05-25 17:38:25,046 |0.0.0.0-startStop-2 |ServiceContainer |I| Removing from cache dispatcher: oagismgr So this one is surely not related to your changes. For the solr issue it's unclear but it does not appear with the test above. And the last change in solr component are the 2016-05-07 (r1742714). This said I don't think those should stop us. They need to be fixed, but not necessarily now... It's always a matter of priorities...
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Also note that my commit r1745451 has an impact, before I got these kind of error (w/ or w/o your changes) when running the "test" or "--test" options with suite or test names:

          Exception in thread "Thread-45" java.lang.RuntimeException: org.ofbiz.minilang.MiniLangException: Could not find SimpleMethod testIfRegexp in XML document in resource: component://minilang/script/org/ofbiz/mi
          nilang/method/ifops/IfRegexpTests.xml
                  at org.ofbiz.minilang.method.ifops.test.IfRegexpTest$MyThread.run(IfRegexpTest.java:82)
          Caused by: org.ofbiz.minilang.MiniLangException: Could not find SimpleMethod testIfRegexp in XML document in resource: component://minilang/script/org/ofbiz/minilang/method/ifops/IfRegexpTests.xml
                  at org.ofbiz.minilang.SimpleMethod.runSimpleMethod(SimpleMethod.java:272)
                  at org.ofbiz.minilang.method.ifops.test.IfRegexpTest$MyThread.run(IfRegexpTest.java:74)
          

          This was maybe Windows only, which did not help. Anyway it's done now!

          Show
          jacques.le.roux Jacques Le Roux added a comment - Also note that my commit r1745451 has an impact, before I got these kind of error (w/ or w/o your changes) when running the "test" or "--test" options with suite or test names: Exception in thread " Thread -45" java.lang.RuntimeException: org.ofbiz.minilang.MiniLangException: Could not find SimpleMethod testIfRegexp in XML document in resource: component: //minilang/script/org/ofbiz/mi nilang/method/ifops/IfRegexpTests.xml at org.ofbiz.minilang.method.ifops.test.IfRegexpTest$MyThread.run(IfRegexpTest.java:82) Caused by: org.ofbiz.minilang.MiniLangException: Could not find SimpleMethod testIfRegexp in XML document in resource: component: //minilang/script/org/ofbiz/minilang/method/ifops/IfRegexpTests.xml at org.ofbiz.minilang.SimpleMethod.runSimpleMethod(SimpleMethod.java:272) at org.ofbiz.minilang.method.ifops.test.IfRegexpTest$MyThread.run(IfRegexpTest.java:74) This was maybe Windows only, which did not help. Anyway it's done now!
          Hide
          taher Taher Alkhateeb added a comment -

          I agree totally, things should be fixed, but one thing at a time as you said.

          My main focus right now is on the startup logic. I believe ofbiz can be much faster not only in startup time but in operation if we untangle all the messy startup logic.

          I think once we sort out all the startup logic and the problems the way multi-threading is handled then many bugs would disappear or get fixed on the way. Multi-threading bugs can lurk for years without being detected. That's why the world is moving to functional languages, but that's another topic altogether.

          Also, I really believe we cannot fix bugs if we don't understand the code, and the code in many places right now is not understandable. There is sooooooooooooooooooooooooo much hidden state that makes things impossible to debug. Refactoring code is the KEY, to even begin tackling these bugs.

          Anyway, good to know I didn't set anything on fire so far

          Show
          taher Taher Alkhateeb added a comment - I agree totally, things should be fixed, but one thing at a time as you said. My main focus right now is on the startup logic. I believe ofbiz can be much faster not only in startup time but in operation if we untangle all the messy startup logic. I think once we sort out all the startup logic and the problems the way multi-threading is handled then many bugs would disappear or get fixed on the way. Multi-threading bugs can lurk for years without being detected. That's why the world is moving to functional languages, but that's another topic altogether. Also, I really believe we cannot fix bugs if we don't understand the code, and the code in many places right now is not understandable. There is sooooooooooooooooooooooooo much hidden state that makes things impossible to debug. Refactoring code is the KEY, to even begin tackling these bugs. Anyway, good to know I didn't set anything on fire so far
          Hide
          taher Taher Alkhateeb added a comment -

          This patch has reference to the thread http://ofbiz.markmail.org/message/b4jovfhurczunbam?q=proposal+to+remove+cobertura+and+sonar+from+ofbiz

          The thread approves (so far) removing cobertura and sonar from the framework to have a leaner cleaner startup code base.

          I ran all the tests and did some smoke tests and nothing is on fire so far. Please check and help if you can.

          Show
          taher Taher Alkhateeb added a comment - This patch has reference to the thread http://ofbiz.markmail.org/message/b4jovfhurczunbam?q=proposal+to+remove+cobertura+and+sonar+from+ofbiz The thread approves (so far) removing cobertura and sonar from the framework to have a leaner cleaner startup code base. I ran all the tests and did some smoke tests and nothing is on fire so far. Please check and help if you can.
          Hide
          jacopoc Jacopo Cappellato added a comment -

          Hi Taher,

          your patch looks good: I have reviewed it and applied to my local OFBiz and all tests pass.
          In the attachment you will find some minor changes that you should include in your commit.

          Show
          jacopoc Jacopo Cappellato added a comment - Hi Taher, your patch looks good: I have reviewed it and applied to my local OFBiz and all tests pass. In the attachment you will find some minor changes that you should include in your commit.
          Hide
          taher Taher Alkhateeb added a comment -

          Nice catch Jacopo, I always forget to do a second grep on hidden files. Or maybe I need to learn a combo to grep everything in one shot.

          Show
          taher Taher Alkhateeb added a comment - Nice catch Jacopo, I always forget to do a second grep on hidden files. Or maybe I need to learn a combo to grep everything in one shot.
          Hide
          taher Taher Alkhateeb added a comment -

          Hi Jacopo,

          The patch you provided breaks in eclipse. I think the reason is that you removed the entire line when you should just remove the "excluding" clause

          In other words, you change from:

          <classpathentry excluding="org/ofbiz/base/config/CoberturaInstrumenter.java" kind="src" path="framework/base/src"/>
          

          to

          <classpathentry kind="src" path="framework/base/src"/>
          
          Show
          taher Taher Alkhateeb added a comment - Hi Jacopo, The patch you provided breaks in eclipse. I think the reason is that you removed the entire line when you should just remove the "excluding" clause In other words, you change from: <classpathentry excluding= "org/ofbiz/base/config/CoberturaInstrumenter.java" kind= "src" path= "framework/base/src" /> to <classpathentry kind= "src" path= "framework/base/src" />
          Hide
          jacopoc Jacopo Cappellato added a comment -

          You are indeed right Taher! I didn't notice it because I don't use Eclipse

          Show
          jacopoc Jacopo Cappellato added a comment - You are indeed right Taher! I didn't notice it because I don't use Eclipse
          Hide
          taher Taher Alkhateeb added a comment -

          Okay fixed everything and committed in r1746236

          I think if we continue on this trend then the start component is going to be so lean it can compete in the Olympics. Okay, I have my eye now on that darned ClassLoader

          Show
          taher Taher Alkhateeb added a comment - Okay fixed everything and committed in r1746236 I think if we continue on this trend then the start component is going to be so lean it can compete in the Olympics. Okay, I have my eye now on that darned ClassLoader
          Hide
          taher Taher Alkhateeb added a comment -

          Hello folks,

          Another patch for refactoring the start component that does the following:

          • convert the startup loaders from ArrayList to List. Given that only two loaders existed in the system since the beginning of OFBiz means that it is really unnecessary to use the loaders.trimToSize(). It's simply too much specificity for no apparent performance value. this leads to changing many method signatures from ArrayList to list across different classes
          • More simplification of Config.java by simplifying timezone setting logic
          • remove the throws StartupException from main. This thing right here confused me a lot and lead to a regression from my refactoring in OFBIZ-7167 because some exceptions bubble up all the way to main but then the system does not terminate because other threads are running. It took me and Jacques a long while to dig it out.
          • Introduce a function called fullyTerminateSystem(StartupException e). This function replaces the exception bubble issue that is described above. Consequently, the methods init, status and shutdown no longer throw an exception but instead terminate the system (which is the right thing to do).
          • finetune adaptStartupCommandsToLoaderArgs(...) and other places in StartupControlPanel
          • separate the Classpath creation from NativeLibClassLoader creation. This makes it cleaner and easier to refactor in the future
          • break the StartupControlPanel.start(...) method into smaller more readable methods. It is easier now to understand the startup sequence for ofbiz in english words (the method names). It also allows for cleaner refactoring in the future.
          • add JavaDoc to a few methods in StartupControlPanel

          Unfortunately, the current version of trunk is failing one of the tests but not related to this patch. So you need to disable the failing test and you will observe that the system works fine. It also works fine if any test fails and does not repeat the regression of OFBIZ-7167. The failing test is testAcctgTransOnEditPoInvoice defined in AutoAcctgTransTestsPurchase.xml

          I feel comfortable about this patch but as usual I wait for your feedback. I think the startup logic is starting to look clear and nice.

          Show
          taher Taher Alkhateeb added a comment - Hello folks, Another patch for refactoring the start component that does the following: convert the startup loaders from ArrayList to List. Given that only two loaders existed in the system since the beginning of OFBiz means that it is really unnecessary to use the loaders.trimToSize(). It's simply too much specificity for no apparent performance value. this leads to changing many method signatures from ArrayList to list across different classes More simplification of Config.java by simplifying timezone setting logic remove the throws StartupException from main. This thing right here confused me a lot and lead to a regression from my refactoring in OFBIZ-7167 because some exceptions bubble up all the way to main but then the system does not terminate because other threads are running. It took me and Jacques a long while to dig it out. Introduce a function called fullyTerminateSystem(StartupException e). This function replaces the exception bubble issue that is described above. Consequently, the methods init, status and shutdown no longer throw an exception but instead terminate the system (which is the right thing to do). finetune adaptStartupCommandsToLoaderArgs(...) and other places in StartupControlPanel separate the Classpath creation from NativeLibClassLoader creation. This makes it cleaner and easier to refactor in the future break the StartupControlPanel.start(...) method into smaller more readable methods. It is easier now to understand the startup sequence for ofbiz in english words (the method names). It also allows for cleaner refactoring in the future. add JavaDoc to a few methods in StartupControlPanel Unfortunately, the current version of trunk is failing one of the tests but not related to this patch. So you need to disable the failing test and you will observe that the system works fine. It also works fine if any test fails and does not repeat the regression of OFBIZ-7167 . The failing test is testAcctgTransOnEditPoInvoice defined in AutoAcctgTransTestsPurchase.xml I feel comfortable about this patch but as usual I wait for your feedback. I think the startup logic is starting to look clear and nice.
          Hide
          taher Taher Alkhateeb added a comment -

          Ok,

          I made one more change but it suddenly made the code much clearer to me and makes more sense.

          What this new patch achieves is everything in the previous patch plus adding a new class called AdminPortClient.

          What is AdminPortClient? It's everything communicated to the OFBiz server through network, not directly.

          This means that everything relating to controlling OFBiz directly happens through StartupControlPanel such as start, stop or init for the server, and everything which happens indirectly through the network (by an admin client) is done through the AdminPortClient (which talks to AdminPortThread)

          This makes the code understandable and really nice on the eyes. Now the whole start component makes sense in a way and you understand the parts of the system and how they talk to each other.

          I look forward to your feedback.

          Show
          taher Taher Alkhateeb added a comment - Ok, I made one more change but it suddenly made the code much clearer to me and makes more sense. What this new patch achieves is everything in the previous patch plus adding a new class called AdminPortClient. What is AdminPortClient? It's everything communicated to the OFBiz server through network, not directly. This means that everything relating to controlling OFBiz directly happens through StartupControlPanel such as start, stop or init for the server, and everything which happens indirectly through the network (by an admin client) is done through the AdminPortClient (which talks to AdminPortThread) This makes the code understandable and really nice on the eyes. Now the whole start component makes sense in a way and you understand the parts of the system and how they talk to each other. I look forward to your feedback.
          Hide
          taher Taher Alkhateeb added a comment -

          one more patch to do everything above but also:

          • replace the name of AdminPortThread with AdminPortServer
          • remove completely any dependencies between AdminPortClient and StartupControlPanel
          Show
          taher Taher Alkhateeb added a comment - one more patch to do everything above but also: replace the name of AdminPortThread with AdminPortServer remove completely any dependencies between AdminPortClient and StartupControlPanel
          Hide
          taher Taher Alkhateeb added a comment -

          I promise this is the last one

          In this patch I update and cleanup the code some more.

          After submitting this patch, I think I'm very close to closing this JIRA

          Show
          taher Taher Alkhateeb added a comment - I promise this is the last one In this patch I update and cleanup the code some more. After submitting this patch, I think I'm very close to closing this JIRA
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          I did not look (yet) at the code, but your comment makes much sense to me!

          Show
          jacques.le.roux Jacques Le Roux added a comment - I did not look (yet) at the code, but your comment makes much sense to me!
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          I created OFBIZ-7192 for that

          Show
          jacques.le.roux Jacques Le Roux added a comment - I created OFBIZ-7192 for that
          Hide
          taher Taher Alkhateeb added a comment -

          In this final patch, I did everything above plus:

          • refactor the AdminPortServer and simplify the logic
          • reorganize the methods in StartupControlPanel
          • document everything

          This JIRA is DONE upon committing this patch. I cannot do anything more without starting to modify other components

          Show
          taher Taher Alkhateeb added a comment - In this final patch, I did everything above plus: refactor the AdminPortServer and simplify the logic reorganize the methods in StartupControlPanel document everything This JIRA is DONE upon committing this patch. I cannot do anything more without starting to modify other components
          Hide
          gil portenseigne Gil Portenseigne added a comment -

          Hi Taher,
          Reading your code is very pleasant, i did test your last patch and did not meet any problems with it, testing several smoke tests.
          The "run tests", without the accounting test (that is not related), show no failure.

          Show
          gil portenseigne Gil Portenseigne added a comment - Hi Taher, Reading your code is very pleasant, i did test your last patch and did not meet any problems with it, testing several smoke tests. The "run tests", without the accounting test (that is not related), show no failure.
          Hide
          jacopoc Jacopo Cappellato added a comment -

          Hi Taher Alkhateeb,

          I am reviewing your last patch and so far it looks really good to me, thank you and congratulations.
          I know this is minor but... what if instead of AdminPortClient and AdminPortServer we use AdminClient and AdminServer?
          These names are more focused on the goal/purpose rather than the implementation detail

          Show
          jacopoc Jacopo Cappellato added a comment - Hi Taher Alkhateeb , I am reviewing your last patch and so far it looks really good to me, thank you and congratulations. I know this is minor but... what if instead of AdminPortClient and AdminPortServer we use AdminClient and AdminServer? These names are more focused on the goal/purpose rather than the implementation detail
          Hide
          taher Taher Alkhateeb added a comment -

          Hi Jacopo,

          Makes perfect sense. I will do that and commit soon. Waiting for Jacques just in case he bumps into something.

          Show
          taher Taher Alkhateeb added a comment - Hi Jacopo, Makes perfect sense. I will do that and commit soon. Waiting for Jacques just in case he bumps into something.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Hi Taher,

          I just reviewed. For the 1st time, a start component review was (almost) straightforward, with useful comments and all. I remember how I was lost the 1st time I naively started to try to understand OFBiz code by reviewing the start class. Kudos, this was not an easy task!

          All works as expected (including the tests and the pos and both options)

          I agree with Jacopo's recommendations, I'd maybe just add a comment about the ofbiz.admin.port use in the tops of the classes. This port and its usage are quite important, so better let's new users not miss it.
          Not sure why you removed commandExistsInList(), found it was more readeable before.
          I'd remove the "StartupControlPanel." prefixes in StartupControlPanel class

          Ah, I just found this line at the end of a tests run or quitting from the POS using the both option

          [java] 2016-06-07 08:57:03,743 FATAL Unable to register shutdown hook because JVM is shutting down.

          but I don't think we should mind since after all the tests and the POS work as expected

          I also noticed something: you can't stop the backend (not POS part) using "ant stop" when you start with "ant start-both" (this was working before, see R15.12), you get

          stop:
               [java] Start.java using configuration file org/ofbiz/base/start/start.properties
               [java] Set OFBIZ_HOME to - C:/projectASF-Mars/ofbiz
               [java] Shutting down server : FAIL
          
          BUILD SUCCESSFUL
          Total time: 1 second
          

          But you can still quit all using the POS, which is fine with me.

          Show
          jacques.le.roux Jacques Le Roux added a comment - Hi Taher, I just reviewed. For the 1st time, a start component review was (almost) straightforward, with useful comments and all. I remember how I was lost the 1st time I naively started to try to understand OFBiz code by reviewing the start class. Kudos, this was not an easy task! All works as expected (including the tests and the pos and both options) I agree with Jacopo's recommendations, I'd maybe just add a comment about the ofbiz.admin.port use in the tops of the classes. This port and its usage are quite important, so better let's new users not miss it. Not sure why you removed commandExistsInList(), found it was more readeable before. I'd remove the "StartupControlPanel." prefixes in StartupControlPanel class Ah, I just found this line at the end of a tests run or quitting from the POS using the both option [java] 2016-06-07 08:57:03,743 FATAL Unable to register shutdown hook because JVM is shutting down. but I don't think we should mind since after all the tests and the POS work as expected I also noticed something: you can't stop the backend (not POS part) using "ant stop" when you start with "ant start-both" (this was working before, see R15.12), you get stop: [java] Start.java using configuration file org/ofbiz/base/start/start.properties [java] Set OFBIZ_HOME to - C:/projectASF-Mars/ofbiz [java] Shutting down server : FAIL BUILD SUCCESSFUL Total time: 1 second But you can still quit all using the POS, which is fine with me.
          Hide
          taher Taher Alkhateeb added a comment -

          Hi Jacques,

          I always feel extra assured after you pitch in. Your QA work is something inspiring

          Anyway, based on your feedback:

          • already renamed all instanced to AdminServer and AdminClient and updated docs
          • Not sure what you mean by adding a comment for ofbiz.admin.port. That's just the property key to fetch the value from in the relevant java properties file. Are you referring to Config.java or the start.properties file or what?
          • I would not worry too much about the details of adaptStartupCommandsToLoaderArgs in StartupControlPanel. My intention is to eventually delete this method completely while also changing the signature of the system containers. This is just a hack to make OFBiz work while switching from String[] to List<StartupCommand>
          • I removed the prefixes, good eyes thank you.
          • It seems the shutting down issue on start-both is due to buggy code outside start. Shutting down the system in the old days with status FAIL means there is a problem. It could be some shutdown sequence in POS or something in the containers, nobody should execute start or shutdown stuff other than the start component. Anyway, I'd rather push that to another day given that the system is functional and we are still heavily refactoring.

          On a side note, I find POS a very annoying component. It's right there in the options of the startup sequence. That's just wrong, it should be another deployable component or alternatively a stand alone application. It should not live inside OFBiz on the startup sequence.

          I will commit everything soon.

          Show
          taher Taher Alkhateeb added a comment - Hi Jacques, I always feel extra assured after you pitch in. Your QA work is something inspiring Anyway, based on your feedback: already renamed all instanced to AdminServer and AdminClient and updated docs Not sure what you mean by adding a comment for ofbiz.admin.port. That's just the property key to fetch the value from in the relevant java properties file. Are you referring to Config.java or the start.properties file or what? I would not worry too much about the details of adaptStartupCommandsToLoaderArgs in StartupControlPanel. My intention is to eventually delete this method completely while also changing the signature of the system containers. This is just a hack to make OFBiz work while switching from String[] to List<StartupCommand> I removed the prefixes, good eyes thank you. It seems the shutting down issue on start-both is due to buggy code outside start. Shutting down the system in the old days with status FAIL means there is a problem. It could be some shutdown sequence in POS or something in the containers, nobody should execute start or shutdown stuff other than the start component. Anyway, I'd rather push that to another day given that the system is functional and we are still heavily refactoring. On a side note, I find POS a very annoying component. It's right there in the options of the startup sequence. That's just wrong, it should be another deployable component or alternatively a stand alone application. It should not live inside OFBiz on the startup sequence. I will commit everything soon.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -
          • for the port, I mean to make it more obvious at 1st glance, but that's not a biggie
          • got it for adaptStartupCommandsToLoaderArgs, I saw the TODO indeed
          • agreed about the FATAL issue (note that it's also with tests)
          • the POS was wrote hastily by Andrew in 2004. He did the fast way... for a customer who never used it... I took it over but never had enough time to rewrite it all right, at least it works right. After discovering I could not build a business on it, I gave up. But it's there and people are using it.
          Show
          jacques.le.roux Jacques Le Roux added a comment - for the port, I mean to make it more obvious at 1st glance, but that's not a biggie got it for adaptStartupCommandsToLoaderArgs, I saw the TODO indeed agreed about the FATAL issue (note that it's also with tests) the POS was wrote hastily by Andrew in 2004. He did the fast way... for a customer who never used it... I took it over but never had enough time to rewrite it all right, at least it works right. After discovering I could not build a business on it, I gave up. But it's there and people are using it.
          Hide
          taher Taher Alkhateeb added a comment -

          Committed all changes in r1747288. This concludes this JIRA, thank you guys for the wonderful support and help.

          Show
          taher Taher Alkhateeb added a comment - Committed all changes in r1747288. This concludes this JIRA, thank you guys for the wonderful support and help.
          Hide
          taher Taher Alkhateeb added a comment -

          This issue might be reopened in case of any regressions, otherwise I mark it as successfully closed and completed.

          Show
          taher Taher Alkhateeb added a comment - This issue might be reopened in case of any regressions, otherwise I mark it as successfully closed and completed.

            People

            • Assignee:
              taher Taher Alkhateeb
              Reporter:
              taher Taher Alkhateeb
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development

                  Agile