Derby
  1. Derby
  2. DERBY-5731

It should be possible to start ij in a mode when it terminates in case of any error

    Details

    • Type: Improvement Improvement
    • Status: In Progress
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Tools
    • Labels:

      Description

      It should be possible to determine from the exit status of ij whether the commands that were executed failed or not. This should not be the default behavior as it will break the compatibility.

      1. derby-5731.diff
        4 kB
        Julius Stroffek

        Activity

        Hide
        Julius Stroffek added a comment -

        I ran into need of this feature and I have already found few posts where people were looking for this kind of stuff, e.g.
        http://osdir.com/ml/apache.db.derby.user/2006-03/msg00131.html

        I would suggest to have the property 'ij.exitOnError' and if that will be assigned to 'true' the method 'handleSQLException' in 'org.apache.derby.impl.tools.ij.utilMain' will throw 'ijFatalException'.

        Show
        Julius Stroffek added a comment - I ran into need of this feature and I have already found few posts where people were looking for this kind of stuff, e.g. http://osdir.com/ml/apache.db.derby.user/2006-03/msg00131.html I would suggest to have the property 'ij.exitOnError' and if that will be assigned to 'true' the method 'handleSQLException' in 'org.apache.derby.impl.tools.ij.utilMain' will throw 'ijFatalException'.
        Hide
        Bryan Pendleton added a comment -

        I think this would be a useful feature to have.

        Show
        Bryan Pendleton added a comment - I think this would be a useful feature to have.
        Hide
        Julius Stroffek added a comment -

        Attaching the patch.

        It creates a new exception ijExitException which is thrown in a similar way than ijFatalException. The further error about exiting ij is dumped in the main function where ijExitException is caught.

        There might be few more things:

        • The attached patch does not exit if there is a failure in connection. However, it exits when there is a statement to be executed on that failed connection.
        • There might be a need for test cases. However, I am not able to find any test cases for ijFatalException either. I am therefore not sure whether the test cases are needed.
        • There is a need to update the documentation with information about ij.exitOnFailure property.

        Please give me some comments to the patch and also please advice what needs to be done before accepting this patch.

        Show
        Julius Stroffek added a comment - Attaching the patch. It creates a new exception ijExitException which is thrown in a similar way than ijFatalException. The further error about exiting ij is dumped in the main function where ijExitException is caught. There might be few more things: The attached patch does not exit if there is a failure in connection. However, it exits when there is a statement to be executed on that failed connection. There might be a need for test cases. However, I am not able to find any test cases for ijFatalException either. I am therefore not sure whether the test cases are needed. There is a need to update the documentation with information about ij.exitOnFailure property. Please give me some comments to the patch and also please advice what needs to be done before accepting this patch.
        Hide
        Knut Anders Hatlen added a comment -

        Hi Julo,

        Thanks for the patch. I haven't looked at it yet, but I have one comment to the described solution:

        A system property that controls the behaviour sounds very reasonable when you start ij from the command line and run it in a separate process. When using the programmatic API (the ij.runScript() method), on the other hand, a system property might not work so well, as it affects the entire JVM, and one may have multiple applications running in the same JVM expecting different behaviour from ij. For such applications it might be better if we add a new ij command, for example called exitonerror, modelled after the localizeddisplay and elapsedtime commands. Then the scripts that wanted this behaviour could start by invoking "exitonerror on" without affecting other applications running ij.runScript() in the same JVM.

        If we add such a command, it probably isn't necessary to add the system property, but we might still want to do that for convenience, I suppose. How do you think an ij command would work for the use case you're struggling with?

        Show
        Knut Anders Hatlen added a comment - Hi Julo, Thanks for the patch. I haven't looked at it yet, but I have one comment to the described solution: A system property that controls the behaviour sounds very reasonable when you start ij from the command line and run it in a separate process. When using the programmatic API (the ij.runScript() method), on the other hand, a system property might not work so well, as it affects the entire JVM, and one may have multiple applications running in the same JVM expecting different behaviour from ij. For such applications it might be better if we add a new ij command, for example called exitonerror, modelled after the localizeddisplay and elapsedtime commands. Then the scripts that wanted this behaviour could start by invoking "exitonerror on" without affecting other applications running ij.runScript() in the same JVM. If we add such a command, it probably isn't necessary to add the system property, but we might still want to do that for convenience, I suppose. How do you think an ij command would work for the use case you're struggling with?
        Hide
        Julius Stroffek added a comment -

        Hi Knut,

        I think exitonerror command might be good. However, I would keep the system property as well. I think that ij command could be used to execute scripts on other JDBC databases (although I have not tested that) and thus the script does not have to be prepared specially for derby. When invoking such a script you could simply set JVM property without altering the script. This is also the case when you would like to run scripts automatically generated from some type of O-R mapping like Hibernate. My use case is execution of Hibernate generated scripts and system property fits better to me.

        For programatic API what you would think about adding an "optional" argument ro ij.runScript that will turn on exit on error. This will avoid usage of derby specific commands in scripts.

        btw: I forgot to exit with the exit code in the patch. I fixed that myself, but I need to change that anyway as it will not work for programatic invocation of ij.runScript.

        Show
        Julius Stroffek added a comment - Hi Knut, I think exitonerror command might be good. However, I would keep the system property as well. I think that ij command could be used to execute scripts on other JDBC databases (although I have not tested that) and thus the script does not have to be prepared specially for derby. When invoking such a script you could simply set JVM property without altering the script. This is also the case when you would like to run scripts automatically generated from some type of O-R mapping like Hibernate. My use case is execution of Hibernate generated scripts and system property fits better to me. For programatic API what you would think about adding an "optional" argument ro ij.runScript that will turn on exit on error. This will avoid usage of derby specific commands in scripts. btw: I forgot to exit with the exit code in the patch. I fixed that myself, but I need to change that anyway as it will not work for programatic invocation of ij.runScript.
        Hide
        Knut Anders Hatlen added a comment -

        Thanks, Julo. Good points, I agree that a system property would be
        useful even if the ij command were implemented.

        Adding a parameter to make runScript() fail-fast sounds like a good
        alternative to the ij command.

        I had a look at the patch, and have a few small comments:

        • Looks like you forgot to include ijExitException.java to the patch,
          so I'm not able to build with it.
        • In utilMain.initFromEnvironment(), converting the property value to
          a boolean should be done using Boolean.valueOf() for consistency,
          like it's done for ij.showErrorCode a few lines
          below. (Boolean.valueOf() is case-insensitive, whereas
          String.equals() is not, and it would be good to handle all boolean
          properties the same way.)
        • Could langUtil and out be changed to parameters of the mainCore()
          method? Having them as static variables might cause problems if two
          threads invoke ij.main() at the same time. (An unlikely way to
          invoke ij, I guess, but since ij.main() is part of the published
          API, one never knows how applications use it.)
        • Because of Derby's API for shutting down databases, which throws an
          exception to signal success, it is quite common for ij scripts to
          invoke statements that are expected to fail:

        ij> connect 'jdbc:derby:db;shutdown=true';
        ERROR 08006: Database 'db' shutdown
        ij> connect 'jdbc:derby:;shutdown=true';
        ERROR XJ015: Derby system shutdown.

        Would it make any sense to add logic to ignore those benign errors?
        Or maybe that would just make the property more difficult to
        understand, I don't know...

        • As to testing, you could probably copy the tools/ij3Test.java test,
          modify it to set the new property, and provide a small test script
          that fails somewhere in the middle (for example by inserting a
          duplicate key into a primary key column, or by executing a statement
          with a syntax error).
        Show
        Knut Anders Hatlen added a comment - Thanks, Julo. Good points, I agree that a system property would be useful even if the ij command were implemented. Adding a parameter to make runScript() fail-fast sounds like a good alternative to the ij command. I had a look at the patch, and have a few small comments: Looks like you forgot to include ijExitException.java to the patch, so I'm not able to build with it. In utilMain.initFromEnvironment(), converting the property value to a boolean should be done using Boolean.valueOf() for consistency, like it's done for ij.showErrorCode a few lines below. (Boolean.valueOf() is case-insensitive, whereas String.equals() is not, and it would be good to handle all boolean properties the same way.) Could langUtil and out be changed to parameters of the mainCore() method? Having them as static variables might cause problems if two threads invoke ij.main() at the same time. (An unlikely way to invoke ij, I guess, but since ij.main() is part of the published API, one never knows how applications use it.) Because of Derby's API for shutting down databases, which throws an exception to signal success, it is quite common for ij scripts to invoke statements that are expected to fail: ij> connect 'jdbc:derby:db;shutdown=true'; ERROR 08006: Database 'db' shutdown ij> connect 'jdbc:derby:;shutdown=true'; ERROR XJ015: Derby system shutdown. Would it make any sense to add logic to ignore those benign errors? Or maybe that would just make the property more difficult to understand, I don't know... As to testing, you could probably copy the tools/ij3Test.java test, modify it to set the new property, and provide a small test script that fails somewhere in the middle (for example by inserting a duplicate key into a primary key column, or by executing a statement with a syntax error).
        Hide
        Dyre Tjeldvoll added a comment -

        Unsetting "Patch available" as the uploaded patch failed to be approved by the reviewer.

        Show
        Dyre Tjeldvoll added a comment - Unsetting "Patch available" as the uploaded patch failed to be approved by the reviewer.

          People

          • Assignee:
            Julius Stroffek
            Reporter:
            Julius Stroffek
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:

              Development