orientechnologies / orientdb-gremlin

TinkerPop3 Graph Structure Implementation for OrientDB
Apache License 2.0
91 stars 32 forks source link

reconnection #54

Closed visox closed 8 years ago

visox commented 8 years ago
mpollmeier commented 8 years ago

just discussed with michal (we're working together) that we'll optimise the check in makeActive to only be triggered after an actual failure. i.e. there won't be any unnecessary calls to the database as it's the case at the moment

velo commented 8 years ago

@mpollmeier nice... keep in mind the pool thing I commented!

visox commented 8 years ago

The history is a bit messy, maybe i can stash the changes.

ALSO: so i tried to make use of the pool that is already there, there was a problem with that since pool.acquire(); created a connection that did not reconnect. So in the current version there is a function PoolRecreate i pass the function to the graph and get a new pool -> new connection. Now to call PoolRecreate means that the pool is mutated in the factory and i am not sure if that is so good, any suggestions ?

mpollmeier commented 8 years ago

@velo any ideas? as michal said the current solution works but it's a bit ugly because it involves mutation, and if anyone is holding onto the pool reference that might get outdated quickly.

I just checked and there seems to be some handling for this directly in orient itself, which would mean we don't actually need to do anything - if only we'd know why it's not being used at the moment... https://github.com/orientechnologies/orientdb/blob/master/client/src/main/java/com/orientechnologies/orient/client/remote/OStorageRemote.java#L1758

visox commented 8 years ago

So i was debugging to see if i go through OStorageRemote, where handleException is defined, and indeed in stacktrace i can find ...at com.orientechnologies.orient.client.remote.OStorageRemote.getPhysicalClusterNameById but that method does not handle the exception, it's not even in the try block that is there, so i guess it's by design (maybe wrong design ?) and up to us to handle that.

velo commented 8 years ago

May be, we should create a wrapper around the pool and in case the ODBPool is returning broken connections we can kill and recreate it...

visox commented 8 years ago

@lvca @laa any ideas/suggestions on this problem ?

in short: we create a OPartitionedDatabasePool pool which can create a ODatabaseDocumentTx that connects fine, then after connection breaks and later works again pool.acquire() still returns a database that is not able to connect. One way is to recreate the whole pool but that is maybe too consuming and adds more complexity and mess to org.apache.tinkerpop.gremlin.orientdb.OrientGraphFactory Last suggestion is to hide the reconnection and pool in a wrap which may reduce complexity and other problems in the client code.

andrii0lomakin commented 8 years ago

@visox Hi I think we may provide way to kill such database instance close. Could you clarify what is result of calling of method database.isClosed() if connection is broken ?

andrii0lomakin commented 8 years ago

I mean in your case of course ))

visox commented 8 years ago

@laa

import com.orientechnologies.orient.client.remote.OServerAdmin;
import com.orientechnologies.orient.core.db.document.ODatabaseDocumentTx;
import com.orientechnologies.orient.server.OServerMain;

public class ODatabaseDocumentTx_isClosed_Test {
    private static String url = "remote:localhost";
    private static String user = "root";
    private static String pass = "somepass";
    private static String dbName = "db-test-failing";
    private static String dbType = "graph";
    private static String dbStorage = "memory";

    private static void createDb() throws Exception {
        new OServerAdmin(url)
                .connect(user, pass)
                .createDatabase(dbName, dbType, dbStorage).close();
    }

    private static void startServer() throws Exception { OServerMain.main(new String[0]); }
    private static void stopServer() { OServerMain.server().shutdown(); }

//config/orientdb-server-config.xml needs to be present for OServerMain

    public static void main(String [] args) throws Exception {
        startServer();
        createDb();

        ODatabaseDocumentTx db = new ODatabaseDocumentTx(url + "/" + dbName);
        db.open(user, pass);

        stopServer();

        try {
            boolean closed = db.isClosed();
            System.out.println(closed);
        } catch (Exception e) {
            e.printStackTrace();
        }
    }
}

false (and no exception)

andrii0lomakin commented 8 years ago

Cool , could you create issue that we have to remove databases with closed connections from the pool . I will provide fix for next 2.1 hotfix ?