You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@activemq.apache.org by franz1981 <gi...@git.apache.org> on 2017/10/05 16:09:16 UTC

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

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 <ni...@gmail.com>
Date:   2017-09-09T16:41:30Z

    ARTEMIS-1447 JDBC NodeManager to support JDBC HA Shared Store

----


---

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

Posted by franz1981 <gi...@git.apache.org>.

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?

---

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

Posted by mtaylor <gi...@git.apache.org>.

Github user mtaylor commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1576
  
    @franz1981 It was an example.  You could instead do what the text is suggesting and just catch the SQLException and re-establish it yourself.  My point is to do the basic thing for the functionality to work, then optimise later.  We don't need a full connection pooling mechanism just to re-establish a connection should one fail.


---

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

Posted by clebertsuconic <gi...@git.apache.org>.

Github user clebertsuconic commented on the issue:

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



---

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

Posted by mtaylor <gi...@git.apache.org>.

Github user mtaylor commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1576
  
    @franz1981 Yes, I meant using a generic connection pool implementations as they reset the connection on release for isolation across components sharing the same pool.  I understand that reconnect is not part of the spec, but it's such a fundamental piece of functionality I'd be surprised if it wasn't already handled in drivers for all our supported DBs.  
    
    I'm not against the idea of using a connection pool (I responded to the comment and missed the fact that you'd implemented your own pool).  This is all good, but is it required for this JIRA?  It's more complexity that may not be required.  For what you're trying to achieve a generic pool would probably suffice.  How about doing something simple for stage 1 to get the HA functionality working.  We can optimise when required.  I would suggest, getting the basic stuff in commit 1, and optimisations in subsequent commits (could be the same PR) but makes it a lot easier to review and track down issues later.


---

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

Posted by clebertsuconic <gi...@git.apache.org>.

Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1576
  
    @franz1981 the ActiveMQScheduledComponent was a suggestion BTW.. do what's right.. even if we need to refactor later...
    
    if you make the impl easier to understand it's fine.


---

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

Posted by clebertsuconic <gi...@git.apache.org>.

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.


---