istreamdata / orientgo

Go (golang) client for OrientDB
MIT License
125 stars 22 forks source link

Travis builds failing #19

Closed ernestas-poskus closed 9 years ago

ernestas-poskus commented 9 years ago

@quux00 There is a pattern for almost all driver commands.

func CreateRecord(dbc *DBClient, doc *oschema.ODocument) error {
    dbc.buf.Reset()

With current implementation it is not possible to use driver asynchronously (no locking as well). Furthermore since there is no locking between commands I guess this is related https://github.com/quux00/ogonori/issues/18 due to err = readStatusCodeAndSessionId(dbc) since where 1 command expects output other writes to same connection..

quux00 commented 9 years ago

I have not fully worked through all aspects of the multi-threaded model for ogonori. However, the intentional design I made early on is that the DBClient is not thread safe. Each user goroutine will need to create its own separate DBClient (or get one from a connection pool once we design that).

There should no shared state between DBClients, so it should be safe to run concurrent code where each goroutine uses its own DBClient. I have a written a simple concurrency test which I just pushed: concurrent_clients.go in package main. You run it with:

./ogonori -c

I ran it a number of times on my Linux system and it passes every time. Let me know what you get.

With current implementation it is not possible to use driver asynchronously (no locking as well)

I'm not quite sure what you mean by "asynchronously". There is support for asynchronous calls to the OrientDB server in the binary network protocol, but ogonori does not support that (and may never support it until I'm convinced there is value in it). I suspect you just mean "concurrent".

Let me know if mean something else or you see a flaw in this design so far.

quux00 commented 9 years ago

Hi Ernestas,

I've burned a few hours trying to get the travis builds to work, without success. All the tests run successfully for me on my Linux system and on my Windows 7 system, with both OrientDB 2.0.1 (Linux) and OrientDB 2.0.14 (Windows). Do they work for you on your local system?

Here's what I've tried and learned so far:

// after connecting the OrientDB server sends back 2 bytes - 
// its binary protocol version
readbuf := make([]byte, 2)
n, err := conx.Read(readbuf)
if err != nil {
    return nil, oerror.NewTrace(err)
}

The connection is getting an EOF back rather than the version (2 bytes) from the OrientDB server. I don't know why and would need a way to log onto the system that is being run and see what the issue is.

My hope was to run a travis env and do the tests locally, say with a Docker set-up. Do you know how to do that? What I saw on this page did not encourage me: https://stackoverflow.com/questions/21053657/how-to-run-travis-ci-locally.

If you have time and interest to get this working, let me know, otherwise I'm going to remove the travis build icon from the README for now.

Thanks for all your help so far.

ernestas-poskus commented 9 years ago

got little overwhelmed by work, this week will contribute more

say with a Docker set-up

I think it is possible to get docker set up

If you have time and interest to get this working, let me know, otherwise I'm going to remove the travis

please don't do that, reasoning behind travis is that we can test driver against many orient db server versions

quux00 commented 9 years ago

If you look at the README you'll see that I commented out the travis build "icon", but I didn't remove it entirely. I haven't removed any of the travis files you created. I agree in general with the testing approach you are pushing for, though I would note that it is still incomplete - it is only testing a single version of OrientDB (we can probably address that) on Linux. No tests are being done on MacOS X or Windows, so it still isn't automating all the testing.

In any case, my view is that the problem right now is (most likely) with the travis setup, not the ogonori code base. So having the "Build Failing" notification on the front page for multiple days/weeks gives the wrong message. In a company we'd have a test server to test out our travis build to get the setup correct and then put it on the "production" one.

-Michael

ernestas-poskus commented 9 years ago

I'm not quite sure what you mean by "asynchronously". There is support for asynchronous calls to the OrientDB server in the binary network protocol, but ogonori does not support that (and may never support it until I'm convinced there is value in it). I suspect you just mean "concurrent".

I meant reusing single dbClient and to do asynchronous queries (asynchronous same as I have not fully worked through all aspects of the multi-threaded model for ogonori ) with current implementation it is only achievable by locking.

E.g.: you wait until lock for connection is released then you take it and lock for next operation and repeat.

Locking for one connection is inefficient.

Furthermore Each user goroutine will need to create its own separate DBClient is not a solution either this might introduce connection leaks & stale connections.

In addition have you thought how to handle cluster connections, failovers, write acknowledgment for different nodes in cluster, write acknowledgment for different quorum settings and distributed transactions ?

We could take examples from gocql (Go Cassandra binary driver) https://github.com/gocql/gocql/blob/1d2c5ebaff56b0ae7ee8f0b6d8069dc0e6188441/cluster.go#L57

P.S. please don't be overwhelmed by my comments you did great job ! I want to help you & myself to get this driver to production. But I want to know if we are on the same track..

ernestas-poskus commented 9 years ago

https://github.com/quux00/ogonori/pull/20

Travis is fixed

quux00 commented 9 years ago

Sorry for the delay in response. I started a new software engineering job on Thursday and have been very busy with that.

Plus, it gave me some time to digest and research your comments. I believe I understand your points now about asynchronous calls.

I have not yet done any ogonori design with that model in mind. Since I am learning OrientDB as I build ogonori, my approach has been to go largely from the bottom up and do the simple things first:

In other words, very much like a JDBC driver for a traditional relational db.

Furthermore Each user goroutine will need to create its own separate DBClient is not a solution either this might introduce connection leaks & stale connections.

I don't agree it's not a valid solution, though it certainly is not the async model you are thinking about. The synchronous, connection pool model is the JDBC and golang database/sql model. Yes, you can have connection leaks if the client code is sloppy. And yes connections can get stale, but a client driver can compensate for that (the Java OrientDB driver does this.)

In addition have you thought how to handle cluster connections, failovers, write acknowledgment for different nodes in cluster, write acknowledgment for different quorum settings and distributed transactions ?

No. All valid points. Beyond a quick read of http://orientdb.com/docs/last/Distributed-Architecture.html#distributed-transactions, I have not studied these areas for OrientDB carefully yet.

We could take examples from gocql (Go Cassandra binary driver) https://github.com/gocql/gocql/blob/1d2c5ebaff56b0ae7ee8f0b6d8069dc0e6188441/cluster.go#L57

I haven't had time to dive into this, but I think that would be a good idea. In addition, my general plan has been to follow the Java OrientDB driver for guidance as much as possible - it has to deal with all of the above.

P.S. please don't be overwhelmed by my comments you did great job ! I want to help you & myself to get this driver to production. But I want to know if we are on the same track.

Thanks for the kind words.

My general view is that we have to crawl before we walk and walk before we run - do one or a few things at a time. That being said, understanding the complex features may help to change the design early on for the better.

So what is the next move?

Mine is to finish getting the simple synchronous single-server transaction code fleshed out. This gets the binary network protocol part for that written and tested.

What do you want to do next? Research the advanced topics and propose a design for how to modify ogonori? A first step might be to understand how to use the OrientDB asynchronous API.

Thanks for helping to advance the project.

ernestas-poskus commented 9 years ago

My general view is that we have to crawl before we walk and walk before we run

:+1:

I would like to focus on connection pool and bytes.Buffer wrapper