search

florinpatrascu / bolt_sips

Neo4j driver for Elixir
Apache License 2.0
258 stars 49 forks source link

Migrate to the DBConnection framework #30

Closed dnesteryuk closed 6 years ago

dnesteryuk commented 7 years ago

This PR is still in progress. The goal is to get feedback before going deeper in changes. :smile:

The main motivation of this migration is to maintain a pool of open connections and keep those connections alive. So, when a query gets executed, the open connection is received from the pool, thus we don't spend time on opening a new connection. A few months ago I created a task related to this PR.

This migration brought a few changes to the api.

The query method requires the pid of the pool process or the pool name.

{:ok, pid} = Bolt.Sips.start_link([])

Bolt.Sips.query!(pid, query)
Bolt.Sips.query!(Bolt.Sips.pool_name, query)

Queries can be executed in a transaction by wrapping them into a function passed to the transaction method:

Bolt.Sips.transaction(pid, fn(conn) ->
  Bolt.Sips.query!(conn, query)
  Bolt.Sips.query!(conn, another_query)

  Bolt.Sips.rollback(conn, "Oops, something went wrong")

  Bolt.Sips.query!(conn, query)
end)

The commit is internally executed by DBConnection. There is no public api for that anymore.

In order to maintain alive connections, the Bolt.Sips.Protocol.ping/1 method closes the open connection each 10 mins (it can be configured via the idle_timeout config option), thus the pool opens a new connection once the old one is closed. It would be perfect to keep the connection if it is still alive, but Neo4j doesn't provide any message (https://boltprotocol.org/v1/#messages) which can be used for that. The doc of the bolt protocol says that connections get closed by clients. So, Neo4j shouldn't close connections, but Neo4j instances offered by https://www.graphenedb.com do close connections in a few hours. At least it is true for the hobby plan.

TODO

Expectation from the review

Thanks! :wink: :+1:

florinpatrascu commented 7 years ago

Hey @dnesteryuk - thank you so much for this PR. You put an impressive amount of work into it, thank you so much! Please allow me few days to review it, as it is a major change, and the time I had available for OS-related works was taken away by the projects I am handling now for the new job which I just started quite recently.

Though, I'd like to make few comments. Mind you, these comments are lacking the thorough review I plan to have when the time will permit, so they may be biased by my lack of fully understanding this PR. Apologize in advance if they're wrong.

I have to admit I had a (failed) attempt to using the DBConnection (for the neo4j_sips project, just b/c everybody was telling me to use it b/c it was cool and every (driver-like) project aiming to claim its seriously, had to use it ... but I wasn't convinced that I need it when I was able to handle what I needed with :poolboy. Probably is the time to reconsider that, and thanks to your work to finally accept I was probably wrong all this time :)

Ok, I'll go back to reviewing and understanding better all these changes and get back to you as soon as possible. Many thanks again!!! - Florin

florinpatrascu commented 7 years ago

Dmitriy - I sent you an invite; this makes the entire process easier. Can you please create a new branch here? You're right, let's use DBConnection, not much use in me resisting to it :) The feature we need to have before merging the code to the master, is: the retry mechanism. Thanks again!

dnesteryuk commented 7 years ago

@florinpatrascu I pushed my changes to this branch. :wink:

Speaking about performance, I noticed performance regression after migrating from Neo4j Sips to Bolt Sips, the performance of my project decreased in 2 times (the response time from my endpoint increased from ~250 msec to ~500 msec). I have to admit, I don't pass an open connection between functions as you suggested in the mentioned task, I just get it when I need to execute a query. But, I gave it a try, I didn't reject your approach right away. :wink: While working on those changes, I saw many lines of code which I had to change, it didn't feel right (probably, it is related to my Ruby experience :blush: ) I thought:

Hey, Elixir is immutable, that is right, but if we need to keep a state we can launch a process and work with that state through that process. Why cannot we delegate handling of connections to some process? So, I can focus on my business logic rather than Neo4j connections. Anyway, there is an agent, it should be ok.

After migrating Bolt Sips to DBConnection, the performance regression was solved. :smile: The response time is around 200 msec.

I will work on the retry feature, but it might take some time. I am a little bit busy with other projects. :smile:

Thanks for this project, Florin! :+1:

florinpatrascu commented 7 years ago

tea/coffee-time reading: https://github.com/elixir-ecto/db_connection/pull/87 - "Requiring a fun for a transaction and non-local return for a rollback is a very limiting API." - I totally agree with this statement :)

fishcakez commented 7 years ago

Oh I saw you referenced an issue. I'm unsure why people were pushing to use DBConnection. It might be more accurately named SQLConnection, or specifically EctoSQLConnection. It's API decisions are largely governed by the postgreSQL wire protocol and Ecto requirements. Its main goal is to provide features and optimisations for Ecto SQL adapters that would be impractical to implement in each adapter. While care is taken to allow other database clients it might be hard to compromise if another client requires a change that is suboptimal for Ecto.Adapters.SQL.

Please note there is a bit of deprecation and churn going on in DBConnection right now so we can get better transaction/cursor/query cache support in Ecto 2.2, now is probably a poor time to adopt it :S. HEAD is currently backwards compatible but a road block might require a 2.0.

dnesteryuk commented 7 years ago

@fishcakez Thank you for your feedback :+1: Personally, I don't have experience in building drivers for DBs. When I started looking into the code of bolt_sips to solve my problem I saw a comment added by @florinpatrascu. I spent some time exploring db_connection, I felt like it gives a solution to a problem I wanted to solve. So, I decided to make this migration. Also, I thought that people who have been contributed to other drivers based on db_connection might contribute to bolt_sips, because it won't be that different. db_connection provides nice abstractions, there are only a few things we need to fill to make it work with Neo4j. It is what I tried to do in this PR. :blush: Do you think I am wrong here?

There is no rush. :wink: I think we can wait for a new release of db_connection which will bring the new interface. It will be even better for bolt_sips' users. We might not introduce breaking changes in the interface.

florinpatrascu commented 7 years ago

Thank you both for the valuable feedback! @fishcakez - would it be premature to ask, if the new DBC version will introduce a higher level of abstraction, so that we could use it outside Ecto for nosql databases; Neo4j, in our case? In our case, with the bolt_sips driver, what attracted us was exactly that: the ability to use various pooling libraries and the fact that the design was done by far more experienced Elixir developers, such as yourself, and the recognition of the value DBC has in the Ecto universe and not only.

Dmitry, I agree with you, we should just wait a bit to see where the new DBC design will take us.

I mean, we could have a perfectly functional Neo4j driver even with the current PR you sent, which it's great, but the retry mechanism and the requirement to use functions in transactions, is what makes me uncomfortable and I don't have a better solution than yours. Probably the next DBC version will improve these aspects. For sure the proposed new design for transactions, the one I mentioned above, addresses my concern, but it is not available yet :)

Many thanks again!!!

fishcakez commented 7 years ago

I didn't look at the implementation. DBConnection has pooling, logging, transactions and an execute function so should allow any nosql usage to be hacked on top if it doesn't fit with the abstraction. A higher level of abstraction might not be helpful because optimisations might be lost, and a driver only needs to implement the callbacks for the functions it uses.

I'm not sure about a built in retry mechanism coupled with transactions because it might be easy for the retry on a in-transaction request to occur outside a transaction. The next release should make it slightly easier to do a retry because it no longer raises an exception unless it really has to on non-bang functions.

leifg commented 7 years ago

Is there any status update on this. I'm seriously interested in this change.

dnesteryuk commented 7 years ago

@leifg The PR is waiting for a new release of DbConnection. The new release will include API we need for this PR.

But if you are eager to use changes provided by this PR, you can set a branch in your mix.exs. I use it for my side project. :smile:

florinpatrascu commented 7 years ago

@leifg - what exactly is in DBConnection that it is so interesting to you? As Dmitriy said, we're looking to see how DBConnection evolves. Not sure about its ETA tho', I'd be ok with merging Dmitriy's PR to master, if I'd know the DBC refactoring is not planned for the near future. In fact, I started to work on adding Connection support for HA, and I am already having issues trying to keep the master and the DBC PR in sync ... Also retrying a connection is a proven advantage of this driver, especially when the Neo4j is hosted on remote machines and the network latency is a sensible aspect; this PR strips out the retry support.

florinpatrascu commented 6 years ago

@fishcakez - James, would you be so kind to give us some feedback about the dbconnection? I still have to decide about this PR, as I want to move forward with adding more feature to my driver here, but kind of blocked b/c I am not sure which way we should go; keep the current implementation, poolboy based, or the one in this PR, using the db_connection?! I was hoping to see the js-no-fun-transaction branch evolving, but its work was deprecated by additional improvements. Would you advise to keep waiting for a newer db_connection? Also, are there any plans to support retries, in db_connection? Thank you, so much!

@dnesteryuk - Dmitriy, I am planning to add bolt routing and HA in the connection, hence my comment above. I really want to bring this driver even closer to the latest Neo4j server capabilities. And once again thank you for your PR, and to @fishcakez for help!

dnesteryuk commented 6 years ago

@florinpatrascu First of all the retry mechanism is already restored. So, here is my plan. I update the PR to work with db_connection from the master which contains new DSL we want. Then you review this PR again and merge it to the master if it is ok. It means bolt_sips will use unreleased version of db_connection for some time. How does it sound? :thinking: If it is ok for you, I can work on it next week, this week I am very busy.

florinpatrascu commented 6 years ago

I wonder if it is not too risky, to use an unreleased (still evolving) dependency? But I'd definitely go with your plan, as I definitely want to start adding the HA and the routing. Plus, even with the PR from you, we'll still have a significant amount of changes to go thru, b/c routing will require the user to specify the type of transactions he plans to execute, i.e. READ, WRITE... I'll try to isolate these changes at the Connection level .. but we'll still need to decorate the transactions with some options .. i.e. R/W. Sure, let's do this. I'll merge the PR in the master, probably tonight or tomorrow, and then we'll make a note in the README, for user to use the master, in case they want to use the db_connection based solution, until we can publish it at hex.pm. Cool! :)

florinpatrascu commented 6 years ago

@dnesteryuk - do you have a conflict free PR? This past week-end I removed the ConCache, as we don't need it anymore. If you don't already have a conflict-free PR or you are too busy, then please let me know and I'll fix this one and merge the result to the master. Thank you!

fishcakez commented 6 years ago

Would you advise to keep waiting for a newer db_connection?

There are plans to move forward with the current approach as 3/4 SQL drivers have the new transactions working and the 4th one requires only a small addition to add support. Hopefully will see something in that regard soon.

Also, are there any plans to support retries, in db_connection?

There are no plans to support retries. Firstly it does not fit very well with transactions because we can not know what needs to be retried, and any side effects that may need to be reversed, without additional user logic.

Secondly I don't think it is the correct place in a stack to support retries. I think retries should be in front of a load balancer so that retries can be redirected to other backends. At the DBConnection level a retry might be likely to fail or cause dangerous load because it only supports connecting to a single host address. Also I think we would want to introduce circuit breaking before retries to help prevent the last issue so we only carry out sane retries.

dnesteryuk commented 6 years ago

@florinpatrascu I am sorry you misunderstood me. I meant next week I will finish this PR and then we will be able to merge it to the master. I would like to avoid merging the uncompleted solution.

florinpatrascu commented 6 years ago

@dnesteryuk - indeed, my bad. My Romanian-English compiler fails me from time to time, sorry. We’ll leave it for when you have time, and I’ll focus on prototyping the connection routing meanwhile. Thank you!

florinpatrascu commented 6 years ago

@fishcakez - James, thank you for the detailed response. Everything makes sense, and we’ll go ahead and adopt the db_connection :) Many thanks to you and the team working on it!

florinpatrascu commented 6 years ago

Ebert has finished reviewing this Pull Request and has found:

You can see more details about this review at https://ebertapp.io/github/florinpatrascu/bolt_sips/pulls/30.

dnesteryuk commented 6 years ago

@florinpatrascu please, have a look again :wink:

Changes:

I tried this branch in my project, it works fine. It will be cool if somebody can try it out too :smile:

florinpatrascu commented 6 years ago

I sure will, thanks so much for all the effort, @dnesteryuk! Awesome!! Will probably start testing this branch toward the mid of the week, or even sooner. 🍻

florinpatrascu commented 6 years ago

Many thanks @dnesteryuk! This PR is in the master branch, and the development will continue from there. Unfortunately publishing this new generation to hex.pm will have to wait until the newest db_connection too is available in hex.pm, as we cannot publish a package using github dependencies.