activemq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From franz1981 <...@git.apache.org>
Subject [GitHub] activemq-artemis issue #1576: ARTEMIS-1447 JDBC NodeManager to support JDBC ...
Date Tue, 10 Oct 2017 08:48:20 GMT
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.
    



---

Mime
View raw message