Karaf
  1. Karaf
  2. KARAF-1572

JDBC Lock without using long running transactions

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.7
    • Fix Version/s: 2.4.0, 3.0.0
    • Component/s: karaf-instance
    • Labels:
      None

      Description

      The current JDBCLock implementations do not work when using Microsoft SQL Server since this requires that you use a CURSOR with the SELECT statement.

      1. GenericJDBCLock.patch
        35 kB
        Claudio Corsi
      2. GenericJDBCLock.patch
        37 kB
        Claudio Corsi

        Activity

        Show
        Guillaume Nodet added a comment - https://git-wip-us.apache.org/repos/asf?p=karaf.git;a=commitdiff;h=411b9e42098529a30642f9fbcd1df5be179437ce https://git-wip-us.apache.org/repos/asf?p=karaf.git;a=commitdiff;h=7a423b219f049549c8983dc44806c331bda9475e
        Hide
        Andreas Pieber added a comment - - edited

        should we really apply this to 2.2.10? Independently of semver.org I think we should stop introducing new features (and therefore new potential bugs) into the quite old 2.2.x branch. Just 0.02€ from my gut feeling

        Show
        Andreas Pieber added a comment - - edited should we really apply this to 2.2.10? Independently of semver.org I think we should stop introducing new features (and therefore new potential bugs) into the quite old 2.2.x branch. Just 0.02€ from my gut feeling
        Hide
        Claudio Corsi added a comment - - edited

        JB, that's fair enough moving this to the next release since the current one is at its final stage.

        As I mentioned to Jamie, I only have a small period in which I can test this against an SQL Server. If you do get a chance to add this feature to one of the branches or trunk and it is ready for testing. I can take some time to set up a test using SQL Server as the jdbc lock.

        Show
        Claudio Corsi added a comment - - edited JB, that's fair enough moving this to the next release since the current one is at its final stage. As I mentioned to Jamie, I only have a small period in which I can test this against an SQL Server. If you do get a chance to add this feature to one of the branches or trunk and it is ready for testing. I can take some time to set up a test using SQL Server as the jdbc lock.
        Hide
        Jean-Baptiste Onofré added a comment -

        As we are really close from 2.2.9 release, I postpone this Jira to 2.2.10.

        Show
        Jean-Baptiste Onofré added a comment - As we are really close from 2.2.9 release, I postpone this Jira to 2.2.10.
        Hide
        Jean-Baptiste Onofré added a comment -

        Thanks Claudio, I gonna take a look.

        Show
        Jean-Baptiste Onofré added a comment - Thanks Claudio, I gonna take a look.
        Hide
        Claudio Corsi added a comment -

        JB, I noticed that you have been distracted with your health and took some time to make some of the changes that Jamie mentioned in one of his comments.

        I have attached the changes for you to look at. The only thing that I did not do is remove the comments as per Jamie but did not add new comments to the new methods I created.

        Hoping that they find out what is wrong and wishing you a quick recovery.

        Show
        Claudio Corsi added a comment - JB, I noticed that you have been distracted with your health and took some time to make some of the changes that Jamie mentioned in one of his comments. I have attached the changes for you to look at. The only thing that I did not do is remove the comments as per Jamie but did not add new comments to the new methods I created. Hoping that they find out what is wrong and wishing you a quick recovery.
        Hide
        Jean-Baptiste Onofré added a comment -

        I gonna review it and enhance the patch. If there is no impact on the existing, it could be acceptable to do it on karaf-2.2.x as well.

        I will keep you posted after some work on that.

        Show
        Jean-Baptiste Onofré added a comment - I gonna review it and enhance the patch. If there is no impact on the existing, it could be acceptable to do it on karaf-2.2.x as well. I will keep you posted after some work on that.
        Hide
        Claudio Corsi added a comment -

        The updates that you are requesting are reasonable. Do you want me to make these changes or do you want to do them?

        I understand that you do not want to add new features to the 2.2.x branch but there is a customer of ours that would like to be able to use SQL Server as a jdbc lock but they are using karaf 2.2.x.

        It would be great if we could include this feature with that branch and/or update the current jdbc lock implementation to be able to use SQL Server.

        Show
        Claudio Corsi added a comment - The updates that you are requesting are reasonable. Do you want me to make these changes or do you want to do them? I understand that you do not want to add new features to the 2.2.x branch but there is a customer of ours that would like to be able to use SQL Server as a jdbc lock but they are using karaf 2.2.x. It would be great if we could include this feature with that branch and/or update the current jdbc lock implementation to be able to use SQL Server.
        Hide
        Jamie goodyear added a comment - - edited

        So I've (and JB) been reviewing the patch, and can see the general direction the code is going in.

        There are a few things I'd like to see modified:

        • remove copy & paste code (acquireLock and stealLock are essentially the same)
        • reduce usage of magic numbers (setInt calls)
        • we try to not set values in main, we read from configs
        • reduce comments, they tend to not be maintained

        Also, I would not call this a default strategy, perhaps nonLongTxLock or the like. Users whom can not use the default strategy can then select the alternative.

        Show
        Jamie goodyear added a comment - - edited So I've (and JB) been reviewing the patch, and can see the general direction the code is going in. There are a few things I'd like to see modified: remove copy & paste code (acquireLock and stealLock are essentially the same) reduce usage of magic numbers (setInt calls) we try to not set values in main, we read from configs reduce comments, they tend to not be maintained Also, I would not call this a default strategy, perhaps nonLongTxLock or the like. Users whom can not use the default strategy can then select the alternative.
        Hide
        Jamie goodyear added a comment -

        Thank you for the patch Claudio.

        As stated before I'm more than happy to have the new mechanism as an option in 2.3.x and 3.0.x lines.

        We're trying our best to not add new features to the 2.2.x line, just bug fixes. That being said if these can live side by side without complications it'd be tempting to see if it can be added to the 2.2.x line, however I'll want to discuss that with the community first (we're really trying to follow the bug fixes only on branch rule).

        Cheers,
        Jamie

        Show
        Jamie goodyear added a comment - Thank you for the patch Claudio. As stated before I'm more than happy to have the new mechanism as an option in 2.3.x and 3.0.x lines. We're trying our best to not add new features to the 2.2.x line, just bug fixes. That being said if these can live side by side without complications it'd be tempting to see if it can be added to the 2.2.x line, however I'll want to discuss that with the community first (we're really trying to follow the bug fixes only on branch rule). Cheers, Jamie
        Hide
        Claudio Corsi added a comment - - edited

        Jamie and Christain,

        Here is the work that I completed that uses a different mechanism to associate a master instance given 2 or more karaf instances. The new jdbc lock feature does not replace the original version but was designed to offer another option to the users. The main difference between this version and the current one is that it does not open a long running transaction and it only uses simple select, update and insert sql commands to manipulate the lock tables. The other change is that it does not use the FOR UPDATE syntax that is not available for all dbms.

        I have not included any tests for this new feature but have tested this successfully using the following dbms.

        • SQL Server
        • MySQL
        • Postgresql
        • Derby

        I have not tried to test it against other dbms but all of the above successfully dealt with a soft and hard failure of karaf. The soft failure is the master karaf instance releasing the lock while the hard failure is the master karaf instance crashing. In both cases, a single karaf instance of the remaining slaves acquired the lock and became the master.

        Note that those tests did not involve starting a karaf instance but instead I created a simple java application that followed the same sequences of calls that the karaf Main class does. I basically extracted those calls in a new class and used that one. This allowed me to stop and start multiple instances of the instances.

        Feel free to look at the code and let me know if this seems to a be viable solution as an addition to the current JDBC lock mechanism.

        Note that the two classes include javadoc explaining how this works and what is expected from the user. Also note that the only difference between the current required properties and this new version is that the new version requires the master instance lock_delay and does not require a timeout value since we are not using a long running transaction.

        I last comment, this work was done against the 2.2.x branch which is currently the 2.2.9 snapshot.

        Show
        Claudio Corsi added a comment - - edited Jamie and Christain, Here is the work that I completed that uses a different mechanism to associate a master instance given 2 or more karaf instances. The new jdbc lock feature does not replace the original version but was designed to offer another option to the users. The main difference between this version and the current one is that it does not open a long running transaction and it only uses simple select, update and insert sql commands to manipulate the lock tables. The other change is that it does not use the FOR UPDATE syntax that is not available for all dbms. I have not included any tests for this new feature but have tested this successfully using the following dbms. SQL Server MySQL Postgresql Derby I have not tried to test it against other dbms but all of the above successfully dealt with a soft and hard failure of karaf. The soft failure is the master karaf instance releasing the lock while the hard failure is the master karaf instance crashing. In both cases, a single karaf instance of the remaining slaves acquired the lock and became the master. Note that those tests did not involve starting a karaf instance but instead I created a simple java application that followed the same sequences of calls that the karaf Main class does. I basically extracted those calls in a new class and used that one. This allowed me to stop and start multiple instances of the instances. Feel free to look at the code and let me know if this seems to a be viable solution as an addition to the current JDBC lock mechanism. Note that the two classes include javadoc explaining how this works and what is expected from the user. Also note that the only difference between the current required properties and this new version is that the new version requires the master instance lock_delay and does not require a timeout value since we are not using a long running transaction. I last comment, this work was done against the 2.2.x branch which is currently the 2.2.9 snapshot.
        Hide
        Christian Schneider added a comment -

        While this might work it sounds a lot more complicated than what we have now. I also do not understand it well enough to be sure it really works reliably. In any case it is really important that when an instance that is active dies another instance will take over. The current implementation makes sure this happens. So I am not sure if it would be a good idea to change the locking like this.

        Still it makes sense that you send the patch. I hope we have some others to join in and discuss your solution.

        Show
        Christian Schneider added a comment - While this might work it sounds a lot more complicated than what we have now. I also do not understand it well enough to be sure it really works reliably. In any case it is really important that when an instance that is active dies another instance will take over. The current implementation makes sure this happens. So I am not sure if it would be a good idea to change the locking like this. Still it makes sense that you send the patch. I hope we have some others to join in and discuss your solution.
        Hide
        Claudio Corsi added a comment - - edited

        It uses two tables, one that is used to define a unique id for each instance and the other is used to determine who the master is.

        The tables are created by a single instance using a transaction that processes more than one sql statements.

        Then each instance generates a unique id by executing the following sql statements.

        step 1 - select id from karaf_node_id
        step 2 - update karaf_node_id set id = curId + 1 where id = curId
        step 3 - repeat step 1 if step was not successful else you have you unique id which is curId + 1

        Then you try to get the master lock by using the following sql statement.

        update karaf_lock set id = unique_id where id = 0 or id = unique_id

        This statement is atomic and only a single instance will succeed removing the need to use transactions.
        Note that there is more to this step than what is here but this is the enhance of it. The where id = unique_id
        is not the complete story. Looking at the implementation will make it clearer.

        I have added other mechanism that handles a heartbeat concept that allows other instance to try to become
        a master if the row was not updated after a given amount of time. Note that this is dependent on the master
        instance heartbeat rate and not on the slave instance heartbeat rate. This is the isAlive method.

        The implementation deals with a master releasing the lock and it the master has a hard failure.

        I will go ahead and cleanup the code with greater description detail on how this is used. As soon as
        that has been done. I will attach the patch to this ticket.

        Show
        Claudio Corsi added a comment - - edited It uses two tables, one that is used to define a unique id for each instance and the other is used to determine who the master is. The tables are created by a single instance using a transaction that processes more than one sql statements. Then each instance generates a unique id by executing the following sql statements. step 1 - select id from karaf_node_id step 2 - update karaf_node_id set id = curId + 1 where id = curId step 3 - repeat step 1 if step was not successful else you have you unique id which is curId + 1 Then you try to get the master lock by using the following sql statement. update karaf_lock set id = unique_id where id = 0 or id = unique_id This statement is atomic and only a single instance will succeed removing the need to use transactions. Note that there is more to this step than what is here but this is the enhance of it. The where id = unique_id is not the complete story. Looking at the implementation will make it clearer. I have added other mechanism that handles a heartbeat concept that allows other instance to try to become a master if the row was not updated after a given amount of time. Note that this is dependent on the master instance heartbeat rate and not on the slave instance heartbeat rate. This is the isAlive method. The implementation deals with a master releasing the lock and it the master has a hard failure. I will go ahead and cleanup the code with greater description detail on how this is used. As soon as that has been done. I will attach the patch to this ticket.
        Hide
        Jamie goodyear added a comment -

        If you have a refactoring of the locking mechanism that would be cool for 2.3.x, 3.x series, I'd try to not make major changes to the general mechanism for 2.2.9, however adding a new backend would be great

        Show
        Jamie goodyear added a comment - If you have a refactoring of the locking mechanism that would be cool for 2.3.x, 3.x series, I'd try to not make major changes to the general mechanism for 2.2.9, however adding a new backend would be great
        Hide
        Christian Schneider added a comment -

        You can add it to this ticket or create a new one both are fine. One question is of course how does you locking work? As far as I know the transaction is the way to make sure you have a relyable lock. So we have to make sure your alternative solution works as well as this.

        Show
        Christian Schneider added a comment - You can add it to this ticket or create a new one both are fine. One question is of course how does you locking work? As far as I know the transaction is the way to make sure you have a relyable lock. So we have to make sure your alternative solution works as well as this.
        Hide
        Claudio Corsi added a comment -

        I was looking at how the current jdbc lock mechanism is implemented and it seems like it isn't generic enough to me.

        I have created something different that uses simple select, insert and update calls without the need to open a long running transaction. I find that to be overkill when the only transaction that should be opened is when you are creating the tables and initial row

        {s}

        .

        I have successfully tested this using derby and will be trying it with other database systems.

        I would like purpose this new jdbc locking mechanism as another option that users can use on top of what you've already implemented.

        The solution does not use the FOR UPDATE requirement which is not necessarily implemented by all databases but uses definitions that are available with all database systems, including SQL Server.

        My question then is, do you want me to include the new feature to this ticket or should I create a new ticket instead?

        Show
        Claudio Corsi added a comment - I was looking at how the current jdbc lock mechanism is implemented and it seems like it isn't generic enough to me. I have created something different that uses simple select, insert and update calls without the need to open a long running transaction. I find that to be overkill when the only transaction that should be opened is when you are creating the tables and initial row {s} . I have successfully tested this using derby and will be trying it with other database systems. I would like purpose this new jdbc locking mechanism as another option that users can use on top of what you've already implemented. The solution does not use the FOR UPDATE requirement which is not necessarily implemented by all databases but uses definitions that are available with all database systems, including SQL Server. My question then is, do you want me to include the new feature to this ticket or should I create a new ticket instead?
        Hide
        Christian Schneider added a comment -

        Can you provide a patch for this? Most of the developers in the Karaf community do not work on windows. I would also like to move this to 3.1.0.

        Show
        Christian Schneider added a comment - Can you provide a patch for this? Most of the developers in the Karaf community do not work on windows. I would also like to move this to 3.1.0.

          People

          • Assignee:
            Guillaume Nodet
            Reporter:
            Claudio Corsi
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development