[GitHub] activemq-artemis pull request #1576: ARTEMIS-1447 JDBC NodeManager to suppor...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
40 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1576: ARTEMIS-1447 JDBC NodeManager to suppor...

pgfox
GitHub user franz1981 opened a pull request:

    https://github.com/apache/activemq-artemis/pull/1576

    ARTEMIS-1447 JDBC NodeManager to support JDBC HA Shared Store

    It implements the JDBC NodeManager to support HA with a JDBC Shared Store.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/franz1981/activemq-artemis ha_jdbc

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/activemq-artemis/pull/1576.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #1576
   
----
commit 3aa6dbfc5b0bad94c8cdcbebfd7ddecd660ac175
Author: Francesco Nigro <[hidden email]>
Date:   2017-09-09T16:41:30Z

    ARTEMIS-1447 JDBC NodeManager to support JDBC HA Shared Store

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1576: ARTEMIS-1447 JDBC NodeManager to support JDBC ...

pgfox
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1576
 
    I will provide soon other commits with the modified NettyFailoverTest tests and docs: I've pushed it now in order to receive the first feedbacks/opinions/reviews :+1:


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1576: ARTEMIS-1447 JDBC NodeManager to suppor...

pgfox
In reply to this post by pgfox
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1576#discussion_r142992370
 
    --- Diff: artemis-server/src/main/resources/schema/artemis-configuration.xsd ---
    @@ -1830,6 +1830,27 @@
                    </xsd:documentation>
                 </xsd:annotation>
              </xsd:element>
    +         <xsd:element name="jdbc-lock-acquisition-timeout" type="xsd:int" minOccurs="0" maxOccurs="1">
    --- End diff --
   
    There is a lot of JDBC config, is it maybe worth making a JDBC config section and type, so its more contained


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1576: ARTEMIS-1447 JDBC NodeManager to suppor...

pgfox
In reply to this post by pgfox
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1576#discussion_r142992780
 
    --- Diff: artemis-server/src/main/resources/schema/artemis-configuration.xsd ---
    @@ -1830,6 +1830,27 @@
                    </xsd:documentation>
                 </xsd:annotation>
              </xsd:element>
    +         <xsd:element name="jdbc-lock-acquisition-timeout" type="xsd:int" minOccurs="0" maxOccurs="1">
    --- End diff --
   
    ignore this, i didn't realise its within a database store type already...helps to read up


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1576: ARTEMIS-1447 JDBC NodeManager to support JDBC ...

pgfox
In reply to this post by pgfox
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1576
 
    @franz1981 is any of this exposed in controls for JDBC, so it can be seen , configured and adjusted via console/management api? Like it is for Journal based.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1576: ARTEMIS-1447 JDBC NodeManager to support JDBC ...

pgfox
In reply to this post by pgfox
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1576
 
    @michaelandrepearce Good point! As soon as this passed the reviews I will make them visible into it, but probably not configurable!


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1576: ARTEMIS-1447 JDBC NodeManager to support JDBC ...

pgfox
In reply to this post by pgfox
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1576
 
    @franz1981 sure makes sense :)


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1576: ARTEMIS-1447 JDBC NodeManager to support JDBC ...

pgfox
In reply to this post by pgfox
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1576
 
    Instead of using the Journal Table for the locks... please create another Table for that.
   
    To be honest... I don't understand what's going on with the data.. and this will be pretty hard to be maintained by someone else if you just use a single table for things like this.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1576: ARTEMIS-1447 JDBC NodeManager to support JDBC ...

pgfox
In reply to this post by pgfox
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1576
 
    for the JdbcNodeManager:
   
    The package server.impl is already bloated.. I tried once to refactor it into smaller packages...
   
    This is a nice abstract model you have, but instead of the big class, cal you create a packet jdbcNodeManager, and add your classes there?
   
   
   
    Also, instead of creating your own scheduledExecutor.. Please extend ActiveMQScheduledComponent... if you can reuse the same executors that we already have created, just pass in the scheduledService as a parameter... and if you must create a new ScheduledService, you can still use the same class for that.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1576: ARTEMIS-1447 JDBC NodeManager to support JDBC ...

pgfox
In reply to this post by pgfox
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1576
 
    ```
          insertStateOnNodeManagerStoreTableSQL = "INSERT INTO " + tableName + " (ID) VALUES (0)";
          insertNodeIdOnNodeManagerStoreTableSQL = "INSERT INTO " + tableName + " (ID) VALUES (3)";
          insertLiveLockOnNodeManagerStoreTableSQL = "INSERT INTO " + tableName + " (ID) VALUES (1)";
          insertBackupLockOnNodeManagerStoreTableSQL = "INSERT INTO " + tableName + " (ID) VALUES (2)";
    ```
   
    Wouldn't be better to have a single statement (Using a separate table as I had already asked).. using prepared statements?
   
    I'm a bit concerned also that the semantic of 1, 2 and 3.. is inside the literal string.. and I see no references on the LeaseLock implementation. I wouldn't understand how to debug this.. it makes it harder to maintain IMO.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1576: ARTEMIS-1447 JDBC NodeManager to support JDBC ...

pgfox
In reply to this post by pgfox
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1576
 
    @clebertsuconic
    > Instead of using the Journal Table for the locks... please create another Table for that.
   
    The node manager is already using a table different from the Journal Table to store the locks/NodeId/state:
   
    ``// Default node manager store table name, used with Database storage type
     private static final String DEFAULT_NODE_MANAGER_STORE_TABLE_NAME = "NODE_MANAGER_STORE";``
   
    > To be honest... I don't understand what's going on with the data.. and this will be pretty hard to be maintained by someone else if you just use a single table for things like this.
   
    You're right, but I've implemented exactly the same data processing (and layout) of [FileLockNodeManager](https://github.com/apache/activemq-artemis/blob/master/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/FileLockNodeManager.java), hence I probably need to document it in both, given that is 1:1 with it: makes sense?
   
    >The package server.impl is already bloated.. I tried once to refactor it into smaller packages...
    This is a nice abstract model you have, but instead of the big class, cal you create a packet jdbcNodeManager, and add your classes there?
   
    Yes, good point: I'll do it :+1:
   
    >Also, instead of creating your own scheduledExecutor.. Please extend ActiveMQScheduledComponent... if you can reuse the same executors that we already have created, just pass in the scheduledService as a parameter... and if you must create a new ScheduledService, you can still use the same class for that.
   
    I'm not sure about it: it's similar to the TimedBuffer flusher thread.
    It (they) needs to not being slowed down by anything in order to work as expected and I've put several checks that will be triggered otherwise.
    If we have already something similar that I can reuse I will do it for sure.
   
    > Wouldn't be better to have a single statement (Using a separate table as I had already asked).. using prepared statements?
    Each line can be used for just one purpose and you can't have more than one line doing it, exactly as the file based NodeManager: IMHO is more dangerous/complex to have a prepared statement here.
   
    >I'm a bit concerned also that the semantic of 1, 2 and 3.. is inside the literal string.. and I see no references on the LeaseLock implementation. I wouldn't understand how to debug this.. it makes it harder to maintain IMO.
   
    Agree :+1:  It is not clear and need more docs as I've answered above on another point.
   
    Considering how it works the only improvement I would do on the data layout is to use 2 different tables,`NODE_MANAGER_STORE_LOCKS` and `NODE_MANAGER_STORE_STATE`, to separate the locks and the shared state data.
    TBH, considering that the first table will have just 2 rows and the second just 1, probably it is not a big improvement that justify the change: keep it simpler probably is a best deal.
   



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1576: ARTEMIS-1447 JDBC NodeManager to support JDBC ...

pgfox
In reply to this post by pgfox
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1576
 
       I'm not sure about it: it's similar to the TimedBuffer flusher thread.
       It (they) needs to not being slowed down by anything in order to work as expected and I've put    
       several checks that will be triggered otherwise.
       If we have already something similar that I can reuse I will do it for sure.
   
    The only reason why we don't use Scheduled Executor at the Timedbuffer is because we use NanoSeconds intervals.. what's not good for the Executor.
   
   
    You already create a ScheduledExecutor... if you need your own executor, still use the component which encapsulates that.. if you can reuse one from the pool.. (It seems you can).. you can just pass in the argument.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1576: ARTEMIS-1447 JDBC NodeManager to support JDBC ...

pgfox
In reply to this post by pgfox
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1576
 
    @clebertsuconic You're right, I need to take a better example to explain what I mean.
    Quoting from the `CriticalAnaylizerImpl`:
   
    ```
          // we are not using any Thread Pool or any Scheduled Executors from the ArtemisServer
          // as that would defeat the purpose,
          // as in any deadlocks the schedulers may be starving for something not responding fast enough
          thread = new Thread("Artemis Critical Analyzer")
    ```
    The reason is similar: if for any reason (eg deadlocks/long JVM pauses/a single long running task) the live/backup locks wouldn't be renewed with the correct timing (ie not a matter to be fast but with a correct timing), the mechanics of the node manager will be compromised. It sounds cathastrofic but is the same limitation of the original JDBC lock of ActiveMQ 6: [LeaseDatabaseLocker](https://github.com/apache/activemq/blob/master/activemq-jdbc-store/src/main/java/org/apache/activemq/store/jdbc/LeaseDatabaseLocker.java ).
    Hence I need 2 executors that cannot be shared with none in order to minimize the chance of being slowed down.
    To summarize:
    > if you need your own executor, still use the component which encapsulates that..
   
    I need 2 of them (live + backup), not shared and to use `scheduleWithFixedRate` instead of `scheduleWithFixedDelay` used in the `ActiveMQScheduledComponent`: probably is not the right choice.
   
    > if you can reuse one from the pool.. (It seems you can).. you can just pass in the argument.
   
    I can pass them in the argument, but given that I need them to be unshared and I can't be sure of it if they will be instantiated elsewhere.
   
    In order to use your advice what I can do (maybe similar) is to pass a common thread pool and run the scheduled executors on top of it.
   
    And Re the package there is a good place (and module) in your opinion where I could move the classes?
   
   
   
   
   
   
   



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1576: ARTEMIS-1447 JDBC NodeManager to support JDBC ...

pgfox
In reply to this post by pgfox
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1576
 
    ActiveMQScheduledComponent can manage its own ScheduledExecutor if you need it... just reuse the class please?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1576: ARTEMIS-1447 JDBC NodeManager to support JDBC ...

pgfox
In reply to this post by pgfox
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1576
 
    @clebertsuconic I have to modify it first: ActiveMQScheduledComponent has only 1 ScheduledExecutor (while i need 2) and it can trigger tasks only with fixed delay while I need to trigger them with a fixed rate.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1576: ARTEMIS-1447 JDBC NodeManager to support JDBC ...

pgfox
In reply to this post by pgfox
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1576
 
    You can just a single one... the ActiveMQScheduledComponent uses an executor... once the timer is reached.. it will call the executor...
   
    I would be careful on adding a lot of threads on the broker...
   
   
    Try reusing the already existent ShceduledExecutor...
   
   
    if you at least reuse the component it's easier to change it IMO.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1576: ARTEMIS-1447 JDBC NodeManager to support JDBC ...

pgfox
In reply to this post by pgfox
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1576
 
    One thing regarding the SQL Provider.. you use the same class for all the statements. Perhaps needs to be refactored...
   
    So, if you for now just avoid constants in your statements.. and isolate the statements well with clear docs.. perhaps we can refactor this in better terms. If you pass in arguments to your statements directly from your internal enumerations it would be easier to understand what's going on.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1576: ARTEMIS-1447 JDBC NodeManager to support JDBC ...

pgfox
In reply to this post by pgfox
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1576
 
    @mtaylor wdyt?
   
    > One thing regarding the SQL Provider.. you use the same class for all the statements. Perhaps needs to be refactored...
   
    > So, if you for now just avoid constants in your statements.. and isolate the statements well with clear docs.. perhaps we can refactor this in better terms. If you pass in arguments to your statements directly from your internal enumerations it would be easier to understand what's going on.



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1576: ARTEMIS-1447 JDBC NodeManager to support JDBC ...

pgfox
In reply to this post by pgfox
Github user mtaylor commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1576
 
    @franz1981 I think using a single class to hold the database statements is fine.  Splitting this up into several classes would mean that every provider would have to override 3 or 4 classes just to change the DB statements.  Unless there's a technical reason to do it, like having to use two tables in a single statement, then I wouldn't bother.
   
    Another note.  I haven't checked these statements work with Derby, but I don't see any overrides in Oracle12C provider.  Please ensure that the tests run and pass with Derby, as this is the default provider in our test suite.  Override them in Oracle12C etc...


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1576: ARTEMIS-1447 JDBC NodeManager to support JDBC ...

pgfox
In reply to this post by pgfox
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1576
 
    @mtaylor
    @clebertsuconic
    > I think using a single class to hold the database statements is fine.
   
    Agree!
    >Another note. I haven't checked these statements work with Derby, but I don't see any overrides in Oracle12C provider. Please ensure that the tests run and pass with Derby, as this is the default provider in our test suite. Override them in Oracle12C etc...
   
    The tests run fine with Derby and MySQL while the other DBMS need futher testing: I've used http://sqlfiddle.com/ to test vs Oracle but is not enough.


---
12