Felix
  1. Felix
  2. FELIX-2280

To much code duplication in DefaultJDBCLock, OracleJDBCLock and MySQLJDBCLock

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: karaf-1.4.0
    • Fix Version/s: karaf 1.6.0
    • Component/s: Karaf
    • Labels:
      None
    • Environment:
      All

      Description

      org.apache.felix.karaf.main.DefaultJDBCLock, org.apache.felix.karaf.main.MySQLJDBCLock and org.apache.felix.karaf.main.OracleJDBCLock has to much code duplications. I propose a solution like in ActiveMQ package org.apache.activemq.store.jdbc.adapter.
      And we should implement some unit tests for it.

      If it's fine for you, I will try to improve this part of karaf and provide a patch for it.

      1. FELIX-2280.patch
        101 kB
        Christian Müller
      2. FELIX-2280.patch
        99 kB
        Christian Müller
      3. FELIX-2280.patch
        99 kB
        Christian Müller
      4. FELIX-2280.patch
        84 kB
        Christian Müller
      5. ASF.LICENSE.NOT.GRANTED--FELIX-2280.patch
        31 kB
        Christian Müller

        Activity

        Hide
        Jamie goodyear added a comment -

        Thanks Christian Müller for supplying the patch

        Show
        Jamie goodyear added a comment - Thanks Christian Müller for supplying the patch
        Hide
        Jamie goodyear added a comment -

        Sending main/src/main/java/org/apache/felix/karaf/main/DefaultJDBCLock.java
        Adding main/src/main/java/org/apache/felix/karaf/main/DerbyJDBCLock.java
        Sending main/src/main/java/org/apache/felix/karaf/main/Lock.java
        Sending main/src/main/java/org/apache/felix/karaf/main/MySQLJDBCLock.java
        Sending main/src/main/java/org/apache/felix/karaf/main/OracleJDBCLock.java
        Sending main/src/main/java/org/apache/felix/karaf/main/Statements.java
        Adding main/src/test/java/org/apache/felix/karaf/main/BaseJDBCLockIntegrationTest.java
        Adding main/src/test/java/org/apache/felix/karaf/main/BaseJDBCLockTest.java
        Adding main/src/test/java/org/apache/felix/karaf/main/DefaultJDBCLockIntegrationTest.java
        Adding main/src/test/java/org/apache/felix/karaf/main/DefaultJDBCLockTest.java
        Adding main/src/test/java/org/apache/felix/karaf/main/DerbyJDBCLockIntegrationTest.java
        Adding main/src/test/java/org/apache/felix/karaf/main/DerbyJDBCLockTest.java
        Adding main/src/test/java/org/apache/felix/karaf/main/MySQLJDBCLockIntegrationTest.java
        Adding main/src/test/java/org/apache/felix/karaf/main/MySQLJDBCLockTest.java
        Adding main/src/test/java/org/apache/felix/karaf/main/OracleJDBCLockIntegrationTest.java
        Adding main/src/test/java/org/apache/felix/karaf/main/OracleJDBCLockTest.java
        Adding main/src/test/java/org/apache/felix/karaf/main/StatementsTest.java
        Transmitting file data .................
        Committed revision 946719.

        Show
        Jamie goodyear added a comment - Sending main/src/main/java/org/apache/felix/karaf/main/DefaultJDBCLock.java Adding main/src/main/java/org/apache/felix/karaf/main/DerbyJDBCLock.java Sending main/src/main/java/org/apache/felix/karaf/main/Lock.java Sending main/src/main/java/org/apache/felix/karaf/main/MySQLJDBCLock.java Sending main/src/main/java/org/apache/felix/karaf/main/OracleJDBCLock.java Sending main/src/main/java/org/apache/felix/karaf/main/Statements.java Adding main/src/test/java/org/apache/felix/karaf/main/BaseJDBCLockIntegrationTest.java Adding main/src/test/java/org/apache/felix/karaf/main/BaseJDBCLockTest.java Adding main/src/test/java/org/apache/felix/karaf/main/DefaultJDBCLockIntegrationTest.java Adding main/src/test/java/org/apache/felix/karaf/main/DefaultJDBCLockTest.java Adding main/src/test/java/org/apache/felix/karaf/main/DerbyJDBCLockIntegrationTest.java Adding main/src/test/java/org/apache/felix/karaf/main/DerbyJDBCLockTest.java Adding main/src/test/java/org/apache/felix/karaf/main/MySQLJDBCLockIntegrationTest.java Adding main/src/test/java/org/apache/felix/karaf/main/MySQLJDBCLockTest.java Adding main/src/test/java/org/apache/felix/karaf/main/OracleJDBCLockIntegrationTest.java Adding main/src/test/java/org/apache/felix/karaf/main/OracleJDBCLockTest.java Adding main/src/test/java/org/apache/felix/karaf/main/StatementsTest.java Transmitting file data ................. Committed revision 946719.
        Hide
        Christian Müller added a comment -

        Good to hear :o)
        Let me know if there is something else what I could do...

        Christian

        Show
        Christian Müller added a comment - Good to hear :o) Let me know if there is something else what I could do... Christian
        Hide
        Jamie goodyear added a comment -

        I've just completed my tests with Oracle, going to spend a little more time with Derby tomorrow, then if all is well commit the patch. Thanks for including the DB failure mode test, much appreciated.

        Oracle has been tricky in the past, so I had to double check in case we had any regressions for FELIX-2072, FELIX-2129, or FELIX-2130. Things look good so far The UNDO tablespace remained essentially constant for a one hour period, and DB service interruptions did cause passing of service between Karaf hosts. The behavior observed when passing "ctrl-D" or "ctrl-C" to a slave process was consistent with Karaf 1.4.0.

        Show
        Jamie goodyear added a comment - I've just completed my tests with Oracle, going to spend a little more time with Derby tomorrow, then if all is well commit the patch. Thanks for including the DB failure mode test, much appreciated. Oracle has been tricky in the past, so I had to double check in case we had any regressions for FELIX-2072 , FELIX-2129 , or FELIX-2130 . Things look good so far The UNDO tablespace remained essentially constant for a one hour period, and DB service interruptions did cause passing of service between Karaf hosts. The behavior observed when passing "ctrl-D" or "ctrl-C" to a slave process was consistent with Karaf 1.4.0.
        Hide
        Jamie goodyear added a comment -

        Thanks for the latest patch, checking it out now

        Show
        Jamie goodyear added a comment - Thanks for the latest patch, checking it out now
        Hide
        Christian Müller added a comment -

        About the issue on 'restoring service after a DB failure':

        I could reproduce this issue with this test (I added two break points and started and stopped the database manually):

            @Test
            public void lockShouldRestoreTheLockAfterADbFailure() throws Exception {
                Lock lock1 = createLock(props);
                assertTrue(lock1.lock());
                assertTrue(lock1.isAlive());
                
                // shut down the database
                
                assertFalse(lock1.isAlive());
                
                // start the database
                
                assertTrue(lock1.lock());
                assertTrue(lock1.isAlive());
            }
        

        I attached a new patch which fixed this issue based on the current trunk.

        Cheers,
        Christian

        Show
        Christian Müller added a comment - About the issue on 'restoring service after a DB failure': I could reproduce this issue with this test (I added two break points and started and stopped the database manually): @Test public void lockShouldRestoreTheLockAfterADbFailure() throws Exception { Lock lock1 = createLock(props); assertTrue(lock1.lock()); assertTrue(lock1.isAlive()); // shut down the database assertFalse(lock1.isAlive()); // start the database assertTrue(lock1.lock()); assertTrue(lock1.isAlive()); } I attached a new patch which fixed this issue based on the current trunk. Cheers, Christian
        Hide
        Christian Müller added a comment -

        About your question to the MySQL grant privileges (I hope i understood it correct):

        To create a new user in MySQL, you could connect to the database with an existing user which has sufficient rights and execute the following commands:

        CREATE USER 'karaf'@'%' IDENTIFIED BY 'password';
        

        This creates the new user 'karaf' with the password 'password'. This user can access the database from every host (because of the '%' sign). Instead of '%', you could also write explicitly the host name or IP address from which you allow to access this database. Than you have to execute the create statement for each host/IP address.

        After creating the user, you have to grant the following rights:

        GRANT SELECT, INSERT, UPDATE, CREATE, LOCK TABLES ON *.* TO 'karaf'@'%';
        

        To execute the unit tests, the database user also needs the 'DROP' privileges!

        You could also grant the privileges on a finer lever. Instead to grant on '.', you could also grant on specific schemata or tables.
        See also http://dev.mysql.com/doc/refman/5.1/en/adding-users.html

        Cheers,
        Christian

        Show
        Christian Müller added a comment - About your question to the MySQL grant privileges (I hope i understood it correct): To create a new user in MySQL, you could connect to the database with an existing user which has sufficient rights and execute the following commands: CREATE USER 'karaf'@'%' IDENTIFIED BY 'password'; This creates the new user 'karaf' with the password 'password'. This user can access the database from every host (because of the '%' sign). Instead of '%', you could also write explicitly the host name or IP address from which you allow to access this database. Than you have to execute the create statement for each host/IP address. After creating the user, you have to grant the following rights: GRANT SELECT, INSERT, UPDATE, CREATE, LOCK TABLES ON *.* TO 'karaf'@'%'; To execute the unit tests, the database user also needs the 'DROP' privileges! You could also grant the privileges on a finer lever. Instead to grant on ' . ', you could also grant on specific schemata or tables. See also http://dev.mysql.com/doc/refman/5.1/en/adding-users.html Cheers, Christian
        Hide
        Christian Müller added a comment -

        About your last question (multiple 'SELECT * FROM tableName FOR UPDATE' in the OracleJDBCLock):

        This behavior is because of this integration test:

            @Test
            public void isAliveShouldReturnFalseIfItNotHoldsTheLock() throws Exception {
                Connection connection = null;
                try {
                    lock = createLock(props);
                    
                    assertTrue(lock.lock());
                    
                    lock.lockConnection.rollback(); // release the lock
                    connection = lock(tableName, clustername); // another connection locks the table
                    
                    assertFalse(lock.isAlive());
                } finally {
                    close(connection);            
                }
            }
        

        I want to make sure, that 'isAlive()' only returns true, if it holds the lock. In this test case, the first instance instance lose the lock and a second instance get it. So, the 'isAlive()' method of the first instance should return false. The only solution I found was to execute 'SELECT FROM UPDATE' each time I check, whether or not this instance holds the lock.

        May be this test is not real or possible? I have adopted it's possible to lose the database lock, but still have a connection to the database...

        Cheers,
        Christian

        Show
        Christian Müller added a comment - About your last question (multiple 'SELECT * FROM tableName FOR UPDATE' in the OracleJDBCLock): This behavior is because of this integration test: @Test public void isAliveShouldReturnFalseIfItNotHoldsTheLock() throws Exception { Connection connection = null ; try { lock = createLock(props); assertTrue(lock.lock()); lock.lockConnection.rollback(); // release the lock connection = lock(tableName, clustername); // another connection locks the table assertFalse(lock.isAlive()); } finally { close(connection); } } I want to make sure, that 'isAlive()' only returns true, if it holds the lock. In this test case, the first instance instance lose the lock and a second instance get it. So, the 'isAlive()' method of the first instance should return false. The only solution I found was to execute 'SELECT FROM UPDATE' each time I check, whether or not this instance holds the lock. May be this test is not real or possible? I have adopted it's possible to lose the database lock, but still have a connection to the database... Cheers, Christian
        Hide
        Jamie goodyear added a comment -

        No worries. I'm still working my way through test cases and reviewing the code - so far happy

        I've noticed that in the OracleJDBCLock updateLock override code that your eventually re-executing "SELECT * FROM tableName FOR UPDATE" as the keep alive mechanism. Do we need to keep instructing Oracle that this is for Update here once we've already captured the table lock? A "SELECT * FROM tableName" should suffice.

        Show
        Jamie goodyear added a comment - No worries. I'm still working my way through test cases and reviewing the code - so far happy I've noticed that in the OracleJDBCLock updateLock override code that your eventually re-executing "SELECT * FROM tableName FOR UPDATE" as the keep alive mechanism. Do we need to keep instructing Oracle that this is for Update here once we've already captured the table lock? A "SELECT * FROM tableName" should suffice.
        Hide
        Christian Müller added a comment -

        Jamie, unfortunately I did not make it because of some other things. I'm in holiday with limited internet access until Sunday. I hope I have some results at the beginning of the next week.
        Sorry,
        Christian

        Show
        Christian Müller added a comment - Jamie, unfortunately I did not make it because of some other things. I'm in holiday with limited internet access until Sunday. I hope I have some results at the beginning of the next week. Sorry, Christian
        Hide
        Jamie goodyear added a comment -

        Many thanks

        I've been testing around with the MySQL backend using the config entry:

        karaf.lock.jdbc.url=jdbc:mysql://127.0.0.1:1527/sample?createDatabaseIfNotExist=true
        

        Is it possible to have MySQL grant privileges to the user/password combo to the database from other machines? When a Master/Slave setup is built a DBA would have to manually grant access to the sample DB.

        Cheers,
        Jamie

        Show
        Jamie goodyear added a comment - Many thanks I've been testing around with the MySQL backend using the config entry: karaf.lock.jdbc.url=jdbc:mysql: //127.0.0.1:1527/sample?createDatabaseIfNotExist= true Is it possible to have MySQL grant privileges to the user/password combo to the database from other machines? When a Master/Slave setup is built a DBA would have to manually grant access to the sample DB. Cheers, Jamie
        Hide
        Christian Müller added a comment -

        Hello Jamie,
        thanks for pointing out this issue. I will have a look on it tomorrow.

        Cheers,
        Christian

        Show
        Christian Müller added a comment - Hello Jamie, thanks for pointing out this issue. I will have a look on it tomorrow. Cheers, Christian
        Hide
        Jamie goodyear added a comment - - edited

        Hi Christian,

        In my testing I came across an issue restoring service after a DB failure. The test case can be reproduced as follows:

        1. Start up DB.
        2. Start first instance of Karaf, configured to DB. This should become the Master.
        3. Start second instance of Karaf, also configured to DB. This should become a Slave.
        4. Stop the DB.
        5. Observe that both instances of Karaf are hung waiting for DB lock.
        6. Start the DB again.
        7. Observe that both instances of Karaf are still hung.

        One of these instances should have become the Master upon return of the DB service. I reproduced this using the Oracle JDBC Lock.

        Cheers,
        Jamie

        Note: The same behavior is observed while using the Default JDBC Lock as well (Derby).

        Show
        Jamie goodyear added a comment - - edited Hi Christian, In my testing I came across an issue restoring service after a DB failure. The test case can be reproduced as follows: 1. Start up DB. 2. Start first instance of Karaf, configured to DB. This should become the Master. 3. Start second instance of Karaf, also configured to DB. This should become a Slave. 4. Stop the DB. 5. Observe that both instances of Karaf are hung waiting for DB lock. 6. Start the DB again. 7. Observe that both instances of Karaf are still hung. One of these instances should have become the Master upon return of the DB service. I reproduced this using the Oracle JDBC Lock. Cheers, Jamie Note: The same behavior is observed while using the Default JDBC Lock as well (Derby).
        Hide
        Jamie goodyear added a comment -

        No problem Christian, this is why we review patches

        I'm still going over the patch, I want to try it out with a few different DBs in various situations.

        I also want to take some time to see why & where each of the messages are being generated in unit tests. They're probably ok, just want to validate it

        Sample output:

        Running org.apache.felix.karaf.main.DefaultJDBCLockTest
        6-May-2010 11:05:59 AM org.apache.felix.karaf.main.DefaultJDBCLock schemaExists
        SEVERE: Error testing for db table: java.lang.NullPointerException
        6-May-2010 11:05:59 AM org.apache.felix.karaf.main.DefaultJDBCLock createSchema
        SEVERE: Could not create schema: java.lang.IllegalStateException: missing behavior definition for the preceeding method call getMetaData()
        6-May-2010 11:05:59 AM org.apache.felix.karaf.main.DefaultJDBCLock updateLock
        WARNING: Failed to update database lock: java.sql.SQLException
        6-May-2010 11:05:59 AM org.apache.felix.karaf.main.DefaultJDBCLock updateLock
        WARNING: Failed to update database lock: java.sql.SQLException
        6-May-2010 11:05:59 AM org.apache.felix.karaf.main.DefaultJDBCLock isAlive
        SEVERE: Lost lock!
        6-May-2010 11:05:59 AM org.apache.felix.karaf.main.DefaultJDBCLock isAlive
        SEVERE: Lost lock!
        6-May-2010 11:05:59 AM org.apache.felix.karaf.main.DefaultJDBCLock updateLock
        WARNING: Failed to update database lock: java.sql.SQLException
        Tests run: 13, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.176 sec
        
        Show
        Jamie goodyear added a comment - No problem Christian, this is why we review patches I'm still going over the patch, I want to try it out with a few different DBs in various situations. I also want to take some time to see why & where each of the messages are being generated in unit tests. They're probably ok, just want to validate it Sample output: Running org.apache.felix.karaf.main.DefaultJDBCLockTest 6-May-2010 11:05:59 AM org.apache.felix.karaf.main.DefaultJDBCLock schemaExists SEVERE: Error testing for db table: java.lang.NullPointerException 6-May-2010 11:05:59 AM org.apache.felix.karaf.main.DefaultJDBCLock createSchema SEVERE: Could not create schema: java.lang.IllegalStateException: missing behavior definition for the preceeding method call getMetaData() 6-May-2010 11:05:59 AM org.apache.felix.karaf.main.DefaultJDBCLock updateLock WARNING: Failed to update database lock: java.sql.SQLException 6-May-2010 11:05:59 AM org.apache.felix.karaf.main.DefaultJDBCLock updateLock WARNING: Failed to update database lock: java.sql.SQLException 6-May-2010 11:05:59 AM org.apache.felix.karaf.main.DefaultJDBCLock isAlive SEVERE: Lost lock! 6-May-2010 11:05:59 AM org.apache.felix.karaf.main.DefaultJDBCLock isAlive SEVERE: Lost lock! 6-May-2010 11:05:59 AM org.apache.felix.karaf.main.DefaultJDBCLock updateLock WARNING: Failed to update database lock: java.sql.SQLException Tests run: 13, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.176 sec
        Hide
        Christian Müller added a comment -

        Hey Jamie! Sorry, the problem was, that I set up the BootstrapLogManager with an empty Properties object. Then Karaf uses its defaults and try to write in the log file '/data/log/karaf.log'. Because this file exists on my machine (I don't know why), I haven't this problem.

        Anyway, I updated the two base unit tests so, that I configure the BootstrapLogManager to use the file 'target/karaf.log' as bootstrap log. This should fix this problem.

        The new patch is again based on the current trunk.

        Cheers,
        Christian

        Show
        Christian Müller added a comment - Hey Jamie! Sorry, the problem was, that I set up the BootstrapLogManager with an empty Properties object. Then Karaf uses its defaults and try to write in the log file '/data/log/karaf.log'. Because this file exists on my machine (I don't know why), I haven't this problem. Anyway, I updated the two base unit tests so, that I configure the BootstrapLogManager to use the file 'target/karaf.log' as bootstrap log. This should fix this problem. The new patch is again based on the current trunk. Cheers, Christian
        Hide
        Jamie goodyear added a comment -

        Hi Christian,

        I'm seeing some errors when I execute "mvn clean install" in karaf after I applied your latest patch to my local checkout. It seems to repeatedly be looking for the karaf log file and failing for each test case. Could you have a quick look?

        Cheers,
        Jamie

        Here is my environment setup in case I'm doing something wrong here:
        jgoodyea@kang:~/x4/karaf/karaf$ mvn -version
        Apache Maven 2.2.1 (r801777; 2009-08-06 16:46:01-0230)
        Java version: 1.6.0_17
        Java home: /opt/tools/jdk1.6.0_17/jre
        Default locale: en_CA, platform encoding: UTF-8
        OS name: "linux" version: "2.6.24-27-generic" arch: "amd64" Family: "unix"

        -------------------------------------------------------
         T E S T S
        -------------------------------------------------------
        Running org.apache.felix.karaf.main.DefaultJDBCLockIntegrationTest
        Tests run: 1, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 0.013 sec
        Running org.apache.felix.karaf.main.DerbyJDBCLockIntegrationTest
        Tests run: 1, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 0.005 sec
        Running org.apache.felix.karaf.main.OracleJDBCLockIntegrationTest
        Tests run: 1, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 0.005 sec
        Running org.apache.felix.karaf.main.DefaultJDBCLockTest
        java.io.FileNotFoundException: /data/log/karaf.log (No such file or directory)
                at java.io.FileOutputStream.openAppend(Native Method)
                at java.io.FileOutputStream.<init>(FileOutputStream.java:177)
                at org.apache.felix.karaf.main.BootstrapLogManager$SimpleFileHandler.open(BootstrapLogManager.java:106)
                at org.apache.felix.karaf.main.BootstrapLogManager$SimpleFileHandler.<init>(BootstrapLogManager.java:95)
                at org.apache.felix.karaf.main.BootstrapLogManager.getDefaultHandler(BootstrapLogManager.java:74)
                at org.apache.felix.karaf.main.DefaultJDBCLock.<init>(DefaultJDBCLock.java:66)
                at org.apache.felix.karaf.main.DefaultJDBCLockTest$2.<init>(DefaultJDBCLockTest.java:64)
                at org.apache.felix.karaf.main.DefaultJDBCLockTest.createConnectionShouldConcatinateOptionsCorrect(DefaultJDBCLockTest.java:64)
        
        Show
        Jamie goodyear added a comment - Hi Christian, I'm seeing some errors when I execute "mvn clean install" in karaf after I applied your latest patch to my local checkout. It seems to repeatedly be looking for the karaf log file and failing for each test case. Could you have a quick look? Cheers, Jamie Here is my environment setup in case I'm doing something wrong here: jgoodyea@kang:~/x4/karaf/karaf$ mvn -version Apache Maven 2.2.1 (r801777; 2009-08-06 16:46:01-0230) Java version: 1.6.0_17 Java home: /opt/tools/jdk1.6.0_17/jre Default locale: en_CA, platform encoding: UTF-8 OS name: "linux" version: "2.6.24-27-generic" arch: "amd64" Family: "unix" ------------------------------------------------------- T E S T S ------------------------------------------------------- Running org.apache.felix.karaf.main.DefaultJDBCLockIntegrationTest Tests run: 1, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 0.013 sec Running org.apache.felix.karaf.main.DerbyJDBCLockIntegrationTest Tests run: 1, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 0.005 sec Running org.apache.felix.karaf.main.OracleJDBCLockIntegrationTest Tests run: 1, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 0.005 sec Running org.apache.felix.karaf.main.DefaultJDBCLockTest java.io.FileNotFoundException: /data/log/karaf.log (No such file or directory) at java.io.FileOutputStream.openAppend(Native Method) at java.io.FileOutputStream.<init>(FileOutputStream.java:177) at org.apache.felix.karaf.main.BootstrapLogManager$SimpleFileHandler.open(BootstrapLogManager.java:106) at org.apache.felix.karaf.main.BootstrapLogManager$SimpleFileHandler.<init>(BootstrapLogManager.java:95) at org.apache.felix.karaf.main.BootstrapLogManager.getDefaultHandler(BootstrapLogManager.java:74) at org.apache.felix.karaf.main.DefaultJDBCLock.<init>(DefaultJDBCLock.java:66) at org.apache.felix.karaf.main.DefaultJDBCLockTest$2.<init>(DefaultJDBCLockTest.java:64) at org.apache.felix.karaf.main.DefaultJDBCLockTest.createConnectionShouldConcatinateOptionsCorrect(DefaultJDBCLockTest.java:64)
        Hide
        Jamie goodyear added a comment -

        Thanks, I'll take a look over this latest patch soon

        Cheers,
        Jamie

        Show
        Jamie goodyear added a comment - Thanks, I'll take a look over this latest patch soon Cheers, Jamie
        Hide
        Christian Müller added a comment -

        Me again...
        I provide a new patch (based on the current trunk) which respects your requirements:

        1) OracleJDBCLock:

        • We perform only a "SELECT FOR UPDATE" in the lock() and in the isAlive() method. This works for Oracle and will hopefully not overflow the UNDO table space for long running instances.

        2) DefaultJDBCLock:

        • Provides the same functionality like the current implementation (add the ";create=true" jdbc url option to a derby connection url, if they is not provided by the user).

        3) DerbyJDBCLock:

        • New Lock implementation for Derby. Also set the ";create=true" jdbc url option, if they is not provided by the user

        4) MySQLJDBCLock:

        • Provides the same functionality like the current implementation, but in a more elegant way (add the ";createDatabaseIfNotExist=true" jdbc url option, if they is not provided by the user).

        I hope that's what you want. Sometime, my brain is a bit slow... :o)

        Cheers,
        Christian

        Show
        Christian Müller added a comment - Me again... I provide a new patch (based on the current trunk) which respects your requirements: 1) OracleJDBCLock: We perform only a "SELECT FOR UPDATE" in the lock() and in the isAlive() method. This works for Oracle and will hopefully not overflow the UNDO table space for long running instances. 2) DefaultJDBCLock: Provides the same functionality like the current implementation (add the ";create=true" jdbc url option to a derby connection url, if they is not provided by the user). 3) DerbyJDBCLock: New Lock implementation for Derby. Also set the ";create=true" jdbc url option, if they is not provided by the user 4) MySQLJDBCLock: Provides the same functionality like the current implementation, but in a more elegant way (add the ";createDatabaseIfNotExist=true" jdbc url option, if they is not provided by the user). I hope that's what you want. Sometime, my brain is a bit slow... :o) Cheers, Christian
        Hide
        Christian Müller added a comment -

        Ok, I got it.
        I have done the changes and work on the unit and integration tests. I hope I can submit the new patch tomorrow.

        Cheers,
        Christian

        Show
        Christian Müller added a comment - Ok, I got it. I have done the changes and work on the unit and integration tests. I hope I can submit the new patch tomorrow. Cheers, Christian
        Hide
        Jamie goodyear added a comment -

        The default JDBCLock should function in the same manner as it does now, as this will minimize the impact on current users

        The default lock's behavior is to fall back onto Derby (it's a good db for testing the locking concept). Under the refactoring I'd like to see this same behavior, obviously though some changes may occur due to the scope of the refactoring being done.

        Cheers,
        Jamie

        Show
        Jamie goodyear added a comment - The default JDBCLock should function in the same manner as it does now, as this will minimize the impact on current users The default lock's behavior is to fall back onto Derby (it's a good db for testing the locking concept). Under the refactoring I'd like to see this same behavior, obviously though some changes may occur due to the scope of the refactoring being done. Cheers, Jamie
        Hide
        Christian Müller added a comment -

        By separating Derby out, I propose the following updates:

        public class DerbyJDBCLock extends DefaultJDBCLock {
        
            public DerbyJDBCLock(Properties props) {
                super(props);
            }
        
            @Override
            Connection createConnection(String driver, String url, String username, String password) throws Exception {
                url = (url.toLowerCase().contains("create=true")) ? url : url + ";create=true";
                
                return super.createConnection(driver, url, username, password);
            }
        }
        

        and we could override the createConnection() method in MySQLJDBCLock like the following:

        public class MySQLJDBCLock extends DefaultJDBCLock {
        
            ...
        
            @Override
            Connection createConnection(String driver, String url, String username, String password) throws Exception {
                url = (url.toLowerCase().contains("createDatabaseIfNotExist=true")) ? 
                    url : 
                    ((url.contains("?")) ? 
                        url + "&createDatabaseIfNotExist=true" : 
                        url + "?createDatabaseIfNotExist=true");
                
                return super.createConnection(driver, url, username, password);
            }
        

        Then, do you would also update the DefaultJDBCLock.createConnection() method to work with derby like now?

        Regards,
        Christian

        Show
        Christian Müller added a comment - By separating Derby out, I propose the following updates: public class DerbyJDBCLock extends DefaultJDBCLock { public DerbyJDBCLock(Properties props) { super (props); } @Override Connection createConnection( String driver, String url, String username, String password) throws Exception { url = (url.toLowerCase().contains( "create= true " )) ? url : url + ";create= true " ; return super .createConnection(driver, url, username, password); } } and we could override the createConnection() method in MySQLJDBCLock like the following: public class MySQLJDBCLock extends DefaultJDBCLock { ... @Override Connection createConnection( String driver, String url, String username, String password) throws Exception { url = (url.toLowerCase().contains( "createDatabaseIfNotExist= true " )) ? url : ((url.contains( "?" )) ? url + "&createDatabaseIfNotExist= true " : url + "?createDatabaseIfNotExist= true " ); return super .createConnection(driver, url, username, password); } Then, do you would also update the DefaultJDBCLock.createConnection() method to work with derby like now? Regards, Christian
        Hide
        Jamie goodyear added a comment - - edited

        Hi Christian,

        The general procedure your following here is ok, however your going to need to make a one off exception for Oracle here

        • 1) check that the connection is not null
        • 2) check that the connection is not closed
        • 3) run a select query against the database
        • 4) check that at leased one row was returned.

        When we perform an update on a long lived locked table Oracle will save a copy of the transaction in it's UNDO table space. Eventually this can cause the UNDO table to become full, disrupting all locks in the DB instance. If you were to commit the update, thereby cleaning up UNDO space, you will give up the table lock. A select query just touches the table, ensuring we can still read the DB but doesn't add to the UNDO.

        Cheers,
        Jamie

        Show
        Jamie goodyear added a comment - - edited Hi Christian, The general procedure your following here is ok, however your going to need to make a one off exception for Oracle here 1) check that the connection is not null 2) check that the connection is not closed 3) run a select query against the database 4) check that at leased one row was returned. When we perform an update on a long lived locked table Oracle will save a copy of the transaction in it's UNDO table space. Eventually this can cause the UNDO table to become full, disrupting all locks in the DB instance. If you were to commit the update, thereby cleaning up UNDO space, you will give up the table lock. A select query just touches the table, ensuring we can still read the DB but doesn't add to the UNDO. Cheers, Jamie
        Hide
        Christian Müller added a comment -

        Jamie, you wrote:
        > One area that will need to be addressed is the isAlive() method. Testing the connection state is not enough, we should attempt a simple query in order to ensure the connection is good (also ensures that the DB itself is healthy).

        My patch try to cover this in the following way:

        • 1) check that the connection is not null
        • 2) check that the connection is not closed
        • 3) run the update query against the database
        • 4) check that at leased one row was updated.

        Only if all checks succeed, isAlive() will return true. Hope that's what you want.

        Regards,
        Christian

        Show
        Christian Müller added a comment - Jamie, you wrote: > One area that will need to be addressed is the isAlive() method. Testing the connection state is not enough, we should attempt a simple query in order to ensure the connection is good (also ensures that the DB itself is healthy). My patch try to cover this in the following way: 1) check that the connection is not null 2) check that the connection is not closed 3) run the update query against the database 4) check that at leased one row was updated. Only if all checks succeed, isAlive() will return true. Hope that's what you want. Regards, Christian
        Hide
        Jamie goodyear added a comment -

        One area that will need to be addressed is the isAlive() method. Testing the connection state is not enough, we should attempt a simple query in order to ensure the connection is good (also ensures that the DB itself is healthy).

        Show
        Jamie goodyear added a comment - One area that will need to be addressed is the isAlive() method. Testing the connection state is not enough, we should attempt a simple query in order to ensure the connection is good (also ensures that the DB itself is healthy).
        Hide
        Jamie goodyear added a comment -

        Many thanks for the patch, I was able to apply it to my local svn checkout

        I'll take a look through the changes and get back to you soon, obviously with a variety of databases a good bit of testing is required (especially around Oracle and its long living lock behaviors).

        The issue of including the jdbc url option for creating a database, we can experiment with the option where the backend database supports it (striving for ease of use is good in my humble opinion).

        I'm ok with separating Derby out from the generic DefaultJDBCLock, long as we can fall back to Derby via the Default – I'd like to avoid having current users need to reconfigure their lock settings.

        As to the other databases you've mentioned please feel free to submit a feature request and patch for any/all you feel like contributing.

        Show
        Jamie goodyear added a comment - Many thanks for the patch, I was able to apply it to my local svn checkout I'll take a look through the changes and get back to you soon, obviously with a variety of databases a good bit of testing is required (especially around Oracle and its long living lock behaviors). The issue of including the jdbc url option for creating a database, we can experiment with the option where the backend database supports it (striving for ease of use is good in my humble opinion). I'm ok with separating Derby out from the generic DefaultJDBCLock, long as we can fall back to Derby via the Default – I'd like to avoid having current users need to reconfigure their lock settings. As to the other databases you've mentioned please feel free to submit a feature request and patch for any/all you feel like contributing.
        Hide
        Christian Müller added a comment -

        Jamie, I attached my patch. It would be nice, if you could have a look on it.
        I use the Apache Git repository and created the patch with Git. With the '--no-prefix' option I use, it should be the same patch format as Subversion creates. I working with the guys from Camel and ServiceMix in the same way and it was not a problem. Hope this works also for you. :o)

        I have also changed the behavior in one point, what you may be dislike. Let me know what you think and I will change this, if you dislike it.
        At present, we append the jdbc url option ';create=true' for derby, if the user doesn't declare it. We could do this also for MySQL (?createDatabaseIfNotExist=true). But because this is a normal jdbc url option, I would leave this decision to the user and document this option in the documentation.
        Otherwise I would prefer to have a concrete DerbyJDBCLock which extends the DefaultJDBCLock and overrides the 'createConnection()' method and add the jdbc url option, if they is not provided. The same for MySQLJDBCLock...

        I have tested successfully the functional tests with the following database and driver versions:

        • Apache Derby Network Server 10.5.3.0 / derbyclient.jar 10.5.3.0_1 (latest version)
        • MySQL Server 5.1.44 / mysql-connector-java.jar 5.1.12 (latest version)
        • Oracle Database 10g Express Edition Release 10.2.0.1.0 / ojdbc14.jar 10.2.0.4.0 (latest version for 10.2)

        I added the '@Ignore' annotation to the integration tests. This ensure, that these tests are not executed. But if somebody sometime would execute the tests, he only have to uncomment the annotation.

        Regards,
        Christian

        P.S.: If you have requests/requirements for additional databases like DB2, Postgress, HSQLDB or other, please let me know. I would like to test and implement the Lock also for this databases...

        Show
        Christian Müller added a comment - Jamie, I attached my patch. It would be nice, if you could have a look on it. I use the Apache Git repository and created the patch with Git. With the '--no-prefix' option I use, it should be the same patch format as Subversion creates. I working with the guys from Camel and ServiceMix in the same way and it was not a problem. Hope this works also for you. :o) I have also changed the behavior in one point, what you may be dislike. Let me know what you think and I will change this, if you dislike it. At present, we append the jdbc url option ';create=true' for derby, if the user doesn't declare it. We could do this also for MySQL (?createDatabaseIfNotExist=true). But because this is a normal jdbc url option, I would leave this decision to the user and document this option in the documentation. Otherwise I would prefer to have a concrete DerbyJDBCLock which extends the DefaultJDBCLock and overrides the 'createConnection()' method and add the jdbc url option, if they is not provided. The same for MySQLJDBCLock... I have tested successfully the functional tests with the following database and driver versions: Apache Derby Network Server 10.5.3.0 / derbyclient.jar 10.5.3.0_1 (latest version) MySQL Server 5.1.44 / mysql-connector-java.jar 5.1.12 (latest version) Oracle Database 10g Express Edition Release 10.2.0.1.0 / ojdbc14.jar 10.2.0.4.0 (latest version for 10.2) I added the '@Ignore' annotation to the integration tests. This ensure, that these tests are not executed. But if somebody sometime would execute the tests, he only have to uncomment the annotation. Regards, Christian P.S.: If you have requests/requirements for additional databases like DB2, Postgress, HSQLDB or other, please let me know. I would like to test and implement the Lock also for this databases...
        Hide
        Christian Müller added a comment -

        My proposal/plan is to update the behavior to the following:

        • new XXXLock() should:
        • create the database, if they not exists - this is possible for derby (create=true) and mysql (createDatabaseIfNotExist=true) with jdbc url properties. We could append this jdbc url properties if they are not provided and let the driver handle the creation of the database.
        • create the lock table if they not exist and populate the table
        • lock() should:
        • return true, if it is still connected, could aquire the lock and could update exact one row
        • otherwise return false
        • isAlive() should:
        • return true, if it is still connected, holds the lock and could update exact one row
        • otherwise return false
        • release() should:
        • release the connection safely, if they still exist
        Show
        Christian Müller added a comment - My proposal/plan is to update the behavior to the following: new XXXLock() should: create the database, if they not exists - this is possible for derby (create=true) and mysql (createDatabaseIfNotExist=true) with jdbc url properties. We could append this jdbc url properties if they are not provided and let the driver handle the creation of the database. create the lock table if they not exist and populate the table lock() should: return true, if it is still connected, could aquire the lock and could update exact one row otherwise return false isAlive() should: return true, if it is still connected, holds the lock and could update exact one row otherwise return false release() should: release the connection safely, if they still exist
        Hide
        Christian Müller added a comment -

        Jamie, good point. Today, I installed an ubuntu 9.10 in my vmWare which hosts my Oracle XE DB. Thanks for the hint...
        P.S. Virtualbox is also a great option.

        Show
        Christian Müller added a comment - Jamie, good point. Today, I installed an ubuntu 9.10 in my vmWare which hosts my Oracle XE DB. Thanks for the hint... P.S. Virtualbox is also a great option.
        Hide
        Jamie goodyear added a comment -

        Sounds good

        I hear you on the running Mac OSX – I developed the first iteration on my mac too, used virtualbox to host an ubuntu 9.04 vm which in turn provided the Oracle XE DB

        Show
        Jamie goodyear added a comment - Sounds good I hear you on the running Mac OSX – I developed the first iteration on my mac too, used virtualbox to host an ubuntu 9.04 vm which in turn provided the Oracle XE DB
        Hide
        Christian Müller added a comment -

        I will provide the patch next week. Because I'm running on Mac OS X, I have to test the Oracle integration test on my pc in my office.

        Show
        Christian Müller added a comment - I will provide the patch next week. Because I'm running on Mac OS X, I have to test the Oracle integration test on my pc in my office.
        Hide
        Jamie goodyear added a comment -

        Regarding "OracleJDBCLock never checks in the alive method, if it holds the lock ", that fix is waiting in a patch already https://issues.apache.org/jira/browse/FELIX-2130

        I agree that the column names should be normalized across all implementation, it's a bit of an artifact from the initial development run Oracle held one of the previous column names as a reserved word, so I just used another one (definitely one of those refactor needed alarms going off there).

        You will also notice that the different DBs will also display slightly different lock behavior, this is undesirable but in some ways necessary due to different locking mechanisms. Lets see what we can do to improve the consistency here

        In case you don't have access to an Oracle DB on hand may I suggest trying to get a copy here:
        http://www.oracle.com/technology/products/database/xe/index.html

        One little gotcha when working with the Oracle DB is that the UNDO table space can become over whelmed during use of long living locks, this accounts for some of the major runtime behavior differences in how Oracle is treated.

        Please keep the questions and patches coming, I'm more than happy to assist where I can

        Show
        Jamie goodyear added a comment - Regarding "OracleJDBCLock never checks in the alive method, if it holds the lock ", that fix is waiting in a patch already – https://issues.apache.org/jira/browse/FELIX-2130 I agree that the column names should be normalized across all implementation, it's a bit of an artifact from the initial development run Oracle held one of the previous column names as a reserved word, so I just used another one (definitely one of those refactor needed alarms going off there). You will also notice that the different DBs will also display slightly different lock behavior, this is undesirable but in some ways necessary due to different locking mechanisms. Lets see what we can do to improve the consistency here In case you don't have access to an Oracle DB on hand may I suggest trying to get a copy here: http://www.oracle.com/technology/products/database/xe/index.html One little gotcha when working with the Oracle DB is that the UNDO table space can become over whelmed during use of long living locks, this accounts for some of the major runtime behavior differences in how Oracle is treated. Please keep the questions and patches coming, I'm more than happy to assist where I can
        Hide
        Jamie goodyear added a comment - - edited

        Hi Christian,

        The last MySQL Connector I had tested with was 'mysql-connector-java-5.0.8-bin.jar', if its failing with the current impl with your mock test then we need to investigate

        Please update Jira with your new patches so I can have a look at the code.

          • edited - removed question on System.getCurrentTimeMillis().

        Cheers,
        Jamie

        Show
        Jamie goodyear added a comment - - edited Hi Christian, The last MySQL Connector I had tested with was 'mysql-connector-java-5.0.8-bin.jar', if its failing with the current impl with your mock test then we need to investigate Please update Jira with your new patches so I can have a look at the code. edited - removed question on System.getCurrentTimeMillis(). Cheers, Jamie
        Hide
        Christian Müller added a comment -

        Hey Jamie,

        good to hear from you. :o)
        You can skip my first patch, because in the mean time I have done some development. i wrote a lot of integration test (against a real database) to discover the actual behavior. After that, I wrote the same tests a unit tests using EasyMock. Than I updated the unit and integration tests to a behavior, what I would expect. At present, I update the three implementations to succeed the tests.
        At the end, I plan to provide two patches. One contains the modified code and the unit tests (using EasyMock). The second patch will include the Integration tests (annotated with @Ignore) which could be useful to test the implementation against a real database with less expenditure.

        I was wondering, that the current implementation of MySQLJDBCLock fails in a simple integration test (using the mysql-connector-java 5.1.12 and mysql server 5.1.44):

        public class MySQLJDBCLockIntegrationTest {

        private MySQLJDBCLock lock1, lock2;
        private Properties props1;

        @Before
        public void setUp()

        { BootstrapLogManager.setProperties(new Properties()); props1 = new Properties(); props1.put("karaf.lock.jdbc.url", "jdbc:mysql://127.0.0.1:3306/test"); props1.put("karaf.lock.jdbc.driver", "com.mysql.jdbc.Driver"); props1.put("karaf.lock.jdbc.user", "root"); props1.put("karaf.lock.jdbc.password", ""); props1.put("karaf.lock.jdbc.table", "LOCK_TABLE_1"); props1.put("karaf.lock.jdbc.clustername", "karaf"); props1.put("karaf.lock.jdbc.timeout", "5"); }

        @Test
        public void lockOnSameCluster() throws Exception

        { lock1 = new MySQLJDBCLock(props1); lock2 = new MySQLJDBCLock(props1); boolean lock1Aquired = lock1.lock(); assertTrue(lock1Aquired); boolean lock2Aquired = lock2.lock(); assertFalse(lock2Aquired); lock1.release(); lock2Aquired = lock2.lock(); assertTrue(lock2Aquired); }

        }

        Regards,
        Christian

        Show
        Christian Müller added a comment - Hey Jamie, good to hear from you. :o) You can skip my first patch, because in the mean time I have done some development. i wrote a lot of integration test (against a real database) to discover the actual behavior. After that, I wrote the same tests a unit tests using EasyMock. Than I updated the unit and integration tests to a behavior, what I would expect. At present, I update the three implementations to succeed the tests. At the end, I plan to provide two patches. One contains the modified code and the unit tests (using EasyMock). The second patch will include the Integration tests (annotated with @Ignore) which could be useful to test the implementation against a real database with less expenditure. I was wondering, that the current implementation of MySQLJDBCLock fails in a simple integration test (using the mysql-connector-java 5.1.12 and mysql server 5.1.44): public class MySQLJDBCLockIntegrationTest { private MySQLJDBCLock lock1, lock2; private Properties props1; @Before public void setUp() { BootstrapLogManager.setProperties(new Properties()); props1 = new Properties(); props1.put("karaf.lock.jdbc.url", "jdbc:mysql://127.0.0.1:3306/test"); props1.put("karaf.lock.jdbc.driver", "com.mysql.jdbc.Driver"); props1.put("karaf.lock.jdbc.user", "root"); props1.put("karaf.lock.jdbc.password", ""); props1.put("karaf.lock.jdbc.table", "LOCK_TABLE_1"); props1.put("karaf.lock.jdbc.clustername", "karaf"); props1.put("karaf.lock.jdbc.timeout", "5"); } @Test public void lockOnSameCluster() throws Exception { lock1 = new MySQLJDBCLock(props1); lock2 = new MySQLJDBCLock(props1); boolean lock1Aquired = lock1.lock(); assertTrue(lock1Aquired); boolean lock2Aquired = lock2.lock(); assertFalse(lock2Aquired); lock1.release(); lock2Aquired = lock2.lock(); assertTrue(lock2Aquired); } } Regards, Christian
        Hide
        Jamie goodyear added a comment - - edited

        Hi Christian,

        I'll take a look over the patch, and post back here.

          • update: Do you have a patch file generated from svn repo?

        Thanks,
        Jamie

        Show
        Jamie goodyear added a comment - - edited Hi Christian, I'll take a look over the patch, and post back here. update: Do you have a patch file generated from svn repo? Thanks, Jamie
        Hide
        Christian Müller added a comment -

        The next thing which I would like to document (if I'm not wrong) is, that we have to use different databases or tables, if we running in a clustered environment (more than one active instances where each running instance has also a standby / slave instance). My first assumption, only to use different cluster names, fails because Derby (may be others also) lock the entire table.

        Show
        Christian Müller added a comment - The next thing which I would like to document (if I'm not wrong) is, that we have to use different databases or tables, if we running in a clustered environment (more than one active instances where each running instance has also a standby / slave instance). My first assumption, only to use different cluster names, fails because Derby (may be others also) lock the entire table.
        Hide
        Christian Müller added a comment -

        The property 'karaf.lock.jdbc.timeout' is never used by 'org.apache.felix.karaf.main.DefaultJDBCLock', 'org.apache.felix.karaf.main.MySQLJDBCLock' or 'org.apache.felix.karaf.main.OracleJDBCLock'.
        Because of lack of documentation, I assume that is the query timeout (statement.setQueryTimeout(timeout)) and not the login timeout (DriverManager.setLoginTimeout(timeout)). Please let me know, if I'm wrong so that I can change my work...

        Show
        Christian Müller added a comment - The property 'karaf.lock.jdbc.timeout' is never used by 'org.apache.felix.karaf.main.DefaultJDBCLock', 'org.apache.felix.karaf.main.MySQLJDBCLock' or 'org.apache.felix.karaf.main.OracleJDBCLock'. Because of lack of documentation, I assume that is the query timeout (statement.setQueryTimeout(timeout)) and not the login timeout (DriverManager.setLoginTimeout(timeout)). Please let me know, if I'm wrong so that I can change my work...
        Hide
        Christian Müller added a comment -

        The attached patch provide the unit tests for these classes, the precondition for the refactoring...
        It makes also 2 little changes in the existing implementations:

        • I refactored 'System.currentTimeMillis()' in its own method to be able to test the generated sql statements
        • I refactored 'Class.forName(driver); DriverManager.getConnection(url, username, password)' in its own method to be able to introduce the mock implemenations

        Looking forward to work on the 'real' patch...

        Show
        Christian Müller added a comment - The attached patch provide the unit tests for these classes, the precondition for the refactoring... It makes also 2 little changes in the existing implementations: I refactored 'System.currentTimeMillis()' in its own method to be able to test the generated sql statements I refactored 'Class.forName(driver); DriverManager.getConnection(url, username, password)' in its own method to be able to introduce the mock implemenations Looking forward to work on the 'real' patch...
        Hide
        Christian Müller added a comment -

        What I dislike on the current solution and what I like to change with my patch is:

        • provide java doc for the interface org.apache.felix.karaf.main.Lock
        • provide unit tests for org.apache.felix.karaf.main.DefaultJDBCLock, org.apache.felix.karaf.main.MySQLJDBCLock, and org.apache.felix.karaf.main.OracleJDBCLock
        • remove the duplicated code in org.apache.felix.karaf.main.DefaultJDBCLock, org.apache.felix.karaf.main.MySQLJDBCLock, and org.apache.felix.karaf.main.OracleJDBCLock
        • all implementations should use the same column names
        • all implementations should work in the same way (what the currently not do in my opinion)
        • OracleJDBCLock never checks in the alive method, if it holds the lock
        • OracleJDBCLock checks also whether the lock tabe exists or not in its constructor
        • OracleJDBCLock dosn't update the lock table in the lock() method

        Detailed description of the current implementations:

        DefaultJDBCLock:
        creation of DefaultJDBCLock
        lock
        -> checks whether the lock tabe exists or not
        -> if table exists
        -> SELECT * FROM LOCK_TABLE FOR UPDATE
        -> UPDATE LOCK_TABLE SET TIME=1 WHERE CLUSTER = 'cluster'
        -> if table not exists
        -> create table LOCK_TABLE (TIME bigint, CLUSTER varchar(20))
        -> insert into LOCK_TABLE (TIME, CLUSTER) values (1, 'cluster')
        -> SELECT * FROM LOCK_TABLE FOR UPDATE
        -> UPDATE LOCK_TABLE SET TIME=1 WHERE CLUSTER = 'cluster'
        release
        -> checks whether connection is open
        -> if its open
        -> rollback connection
        -> close connection
        isAlive
        -> checks whether connection is open
        -> if its open
        -> UPDATE LOCK_TABLE SET TIME=timestamp WHERE CLUSTER = 'cluster'
        -> checks the updated row count
        -> if is lower then 1
        -> return false
        -> otherwise
        -> return true
        -> if its closed
        -> return false

        OracleJDBCLock:
        creation of OracleJDBCLock
        -> checks whether the lock tabe exists or not
        -> if table not exists
        -> create table LOCK_TABLE (MOMENT number(20), NODE varchar2(20))
        -> insert into LOCK_TABLE (MOMENT, NODE) values (timestamp, 'cluster')
        lock
        -> checks whether the lock tabe exists or not
        -> if table exists
        -> SELECT * FROM LOCK_TABLE FOR UPDATE NOWAIT
        -> SELECT * FROM LOCK_TABLE
        release
        -> checks whether connection is open
        -> if its open
        -> rollback connection
        -> close connection
        isAlive
        -> checks whether connection is open
        -> if its open
        -> return true
        -> if its closed
        -> return false

        MySQLJDBCLock:
        creation of MySQLJDBCLock
        -> create database if not exists karaf
        lock
        -> checks whether the lock tabe exists or not
        -> if table exists
        -> LOCK TABLES karaf.LOCK_TABLE WRITE
        -> UPDATE LOCK_TABLE SET TIME=timestamp WHERE CLUSTER = 'cluster'
        -> if table not exists
        -> create table LOCK_TABLE (TIME bigint, CLUSTER varchar(20)) ENGINE = INNODB
        -> insert into LOCK_TABLE (TIME, CLUSTER) values (timestamp, 'cluster')
        -> LOCK TABLES karaf.LOCK_TABLE WRITE
        -> UPDATE LOCK_TABLE SET TIME=timestamp WHERE CLUSTER = 'cluster'
        release
        -> checks whether connection is open
        -> if its open
        -> rollback connection
        -> close connection
        isAlive
        -> checks whether connection is open
        -> if its open
        -> UPDATE LOCK_TABLE SET TIME=timestamp WHERE CLUSTER = 'cluster'
        -> checks the updated row count
        -> if is lower then 1
        -> return false
        -> otherwise
        -> return true
        -> if its closed
        -> return false

        Show
        Christian Müller added a comment - What I dislike on the current solution and what I like to change with my patch is: provide java doc for the interface org.apache.felix.karaf.main.Lock provide unit tests for org.apache.felix.karaf.main.DefaultJDBCLock, org.apache.felix.karaf.main.MySQLJDBCLock, and org.apache.felix.karaf.main.OracleJDBCLock remove the duplicated code in org.apache.felix.karaf.main.DefaultJDBCLock, org.apache.felix.karaf.main.MySQLJDBCLock, and org.apache.felix.karaf.main.OracleJDBCLock all implementations should use the same column names all implementations should work in the same way (what the currently not do in my opinion) OracleJDBCLock never checks in the alive method, if it holds the lock OracleJDBCLock checks also whether the lock tabe exists or not in its constructor OracleJDBCLock dosn't update the lock table in the lock() method Detailed description of the current implementations: DefaultJDBCLock: creation of DefaultJDBCLock lock -> checks whether the lock tabe exists or not -> if table exists -> SELECT * FROM LOCK_TABLE FOR UPDATE -> UPDATE LOCK_TABLE SET TIME=1 WHERE CLUSTER = 'cluster' -> if table not exists -> create table LOCK_TABLE (TIME bigint, CLUSTER varchar(20)) -> insert into LOCK_TABLE (TIME, CLUSTER) values (1, 'cluster') -> SELECT * FROM LOCK_TABLE FOR UPDATE -> UPDATE LOCK_TABLE SET TIME=1 WHERE CLUSTER = 'cluster' release -> checks whether connection is open -> if its open -> rollback connection -> close connection isAlive -> checks whether connection is open -> if its open -> UPDATE LOCK_TABLE SET TIME=timestamp WHERE CLUSTER = 'cluster' -> checks the updated row count -> if is lower then 1 -> return false -> otherwise -> return true -> if its closed -> return false OracleJDBCLock: creation of OracleJDBCLock -> checks whether the lock tabe exists or not -> if table not exists -> create table LOCK_TABLE (MOMENT number(20), NODE varchar2(20)) -> insert into LOCK_TABLE (MOMENT, NODE) values (timestamp, 'cluster') lock -> checks whether the lock tabe exists or not -> if table exists -> SELECT * FROM LOCK_TABLE FOR UPDATE NOWAIT -> SELECT * FROM LOCK_TABLE release -> checks whether connection is open -> if its open -> rollback connection -> close connection isAlive -> checks whether connection is open -> if its open -> return true -> if its closed -> return false MySQLJDBCLock: creation of MySQLJDBCLock -> create database if not exists karaf lock -> checks whether the lock tabe exists or not -> if table exists -> LOCK TABLES karaf.LOCK_TABLE WRITE -> UPDATE LOCK_TABLE SET TIME=timestamp WHERE CLUSTER = 'cluster' -> if table not exists -> create table LOCK_TABLE (TIME bigint, CLUSTER varchar(20)) ENGINE = INNODB -> insert into LOCK_TABLE (TIME, CLUSTER) values (timestamp, 'cluster') -> LOCK TABLES karaf.LOCK_TABLE WRITE -> UPDATE LOCK_TABLE SET TIME=timestamp WHERE CLUSTER = 'cluster' release -> checks whether connection is open -> if its open -> rollback connection -> close connection isAlive -> checks whether connection is open -> if its open -> UPDATE LOCK_TABLE SET TIME=timestamp WHERE CLUSTER = 'cluster' -> checks the updated row count -> if is lower then 1 -> return false -> otherwise -> return true -> if its closed -> return false

          People

          • Assignee:
            Jamie goodyear
            Reporter:
            Christian Müller
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development