Boltex about bolt_sips HOT 24 CLOSED

Hi @mschae,

I really, and honestly, appreciate your message!

I took the decision of importing the Boltex code, when I realized you're planning to add pooling support, and DB Connection it simply didn't cut it for me. Trust me, I tried tinkering at it and failed to obtain a stable result - spent too many hours on that, before giving up. Your plan to add pooling is/was definitely justified, as there is very little use for a driver w/o pooling and resource management support, so I completely understood your design strategy. But in my case I already had that, and now I also have the ability to retry failing requests i.e. timeouts, busy server, etc, and fail graciously when needed - these too also supported outside the Boltex area of concerns and responsibilities.

If you envision refactoring Boltex into a separate library, for say: encode/decode responsibilities, the very low level Bolt protocol implementation, and separate the resource management i.e. pooling, into a different package, then by all means I'd love to use Boltex from its mothership :) Of course there is a big disadvantage in forking projects, a decision I din't take lightly, in this case - but one that allowed me to insulate my driver from the DB Connection, and allow my users to using Bolt connections w/o risks, while I also was able to move forward and improve my own driver.

Looking forward to hearing back from you.

Hope this helps, and thanks again for sharing your thoughts
Florin

from bolt_sips.

@florinpatrascu,

Thanks for your awesome response. I was hoping that what you described would be the case.

I still believe that shipping a very low level driver like Boltex with some kind of connection management is a good idea. This stems solely from copying other projects like Postgrex.

However I would love to find a way where the DBConnection part of Boltex was "use it if you want it" (or, in other words, make it completely optional to use). That would allow users like myself who use a low level implementation to use this standardised connection management while allowing mature projects like this one to use the low level functionality only (as you currently do).

If you strongly object against having both in the same project I'd rather add a second project for the DBConnection than see this diverge further. So while I prefer option A, I'll work with option B as well. :)

Unfortunately the db-connection branch does not only have DB connections but other fixes in there (especially around error handling). However those should be the only API-breaking changes (returning {:error, %Boltex.Error{}} instead of {:error, :reason}. I'm willing to roll that back, too.

Otherwise the db-connection branch should be usable from BoltSips as I intended DBConnection to only be an optional add-on from the get go. If that is not the case I'm willing to refactor the DBConnection branch so that it is backwards compatible.

As for timeout management, etc. it's obvious that boltex is currently very alpha. Your implementation of user-definable timeouts and error management is something I'd love to merge back into boltex (with your permission).

Thanks again for your response, I'm looking forward to figuring out a way forward together and work with you on making this future proof while continuously usable for your project!

Thanks,
Michael

from bolt_sips.

@mschae - thank you again for support. I would prefer to not needing importing DB Connection w/ Boltex, if that's not too much asking?! I too originally got caught in trying to copy Postgrex (learning from the best, eh?! :), only to fall on my own sword shortly after that (and many sleepless nights); partly b/c I am not an Elixir master, and 2nd: b/c at the end I found it to be an over-engineered solution for Neo4j. While it works for a relational db (Postgresql, in this case) Neo4j doesn't need support for pre-compiled statements or the need to store the Query in a special wrapper with the Response and its params/etc, and so on. It may be my superficiality, but I am very happy with the simple yet very robust pool-boy based pooling I have right now, and never regretted the decision. If you can separate the DB Connection from Boltex, then please don't lose the fixes too, I have no problem refactoring my code to using %Boltex.Error{} instead. I am anyway planning a 0.2.X - and migrate my driver to Elixir 1.4.

Regarding the time-out and retry management, you definitely don't need my permission for merging the approach in your own projects, please feel free to do so. After trying my own solution, I found a hidden trove with gems, no pun intended: retry, and decided to scrap my own implementation in favour of the latter; a more elegant implementation than mine.

Let me know when I should start using the Boltex from hex.pm ;) I'd be very happy to do that.

Many thanks!!
Florin

from bolt_sips.

Oh, and @mschae, if you plan to use :retry (or similar strategies), can you please make it optional for Boltex itself? I personally believe that's the responsibility of the upper resource management layer, the one where, in your case, you'd probably want to use Boltex w/ DB Connection, for example?! For me, Boltex should just reflect back the truth as reported by the Neo4j server and permit the upper (Transaction) managers handling the retries, pooling and so on. A solid separation of roles and responsibilities, between the low-level driver and client code. In my case not only that I need to retry the Bolt request, but I also have to retry the client code accompanying the request. I hope I don't intrude too much in your design?! Thank you!

from bolt_sips.

Hey,

I think your points are fair. I came to a different decision regarding DBConnection but then again, your points are valid and we can agree to disagree. :)

I concur that anything related to retrying etc. should be handled upstream. I will concentrate on returning the best possible error messages, the rest is up to the implementation.

There is one more breaking change: I expect an empty Tuple {} instead of nil when we don't want to authenticate agains Neo4J.

All the fixes and the error handling are now in mschae/boltex#11 - I have yet to add a better way of handling timeouts. I am thinking Mix Configs:

# in config/config.ex

config :boltex, 
  recv_timeout: 10_000

Would that work for you? Alternatively we could add options to the corresponding functions (init, run_statement, etc.

from bolt_sips.

and we can agree to disagree. :)
👍

Alternatively we could add options to the corresponding functions (init, run_statement, etc.

I'd prefer this, if possible?! Since most probably I'd have to pass my timeout config to Boltex, since I am anyway maintaining one as well, in my own config.

from bolt_sips.

There is one more breaking change: I expect an empty Tuple {} instead of nil when we don't want to authenticate agains Neo4J.

no problem :)

from bolt_sips.

Yeah that makes sense. I'll add those options tomorrow. I think I'd prefer it that way, too, when I think more about it.

I'll ping you here when I added the options, afterwards the PR mentioned above should be ready for you to test. Let me know if there is anything I can do to help or if I'm missing something you need.

Thanks for working on this with me. I'm very happy that we can consolidate our resources on one driver and I'm sure all of our projects will benefit. Go OSS! :)

from bolt_sips.

Same here, thank you Michael! Go OSS, indeed :)

from bolt_sips.

Hey @florinpatrascu,

I believe that the add-dbconnection branch now supports everything you need - and nothing more! :) Don't be confused by the name, I was too lazy to open up a new PR.

It would be great if you could try that branch out and see if everything is working correctly for you.

Thank you,
Michael

from bolt_sips.

Hi Michael - this is awesome. I'll start testing and integrating it as soon as I get home from my day job :) And I'll ping here, with feedback. Many thanks!

from bolt_sips.

Michael - last night I started to refactor my code and currently adapting it to your new error management strategy. Before continuing, I wonder if you shouldn't call ack_failure before returning any errors?!

from bolt_sips.

That's a fair question.

I am going to say "no" because I believe catching and recovering from errors should be handled upstream, but whoever manages the connections.

Here's an example how the DBConnection implementation handles that:
https://github.com/mschae/boltex_db_connection/blob/master/lib/boltex_db_connection/connection.ex#L57-L70

We could make it part of the protocol driver but given how low-level we decided to make this, I am in leaning in favour of handling it in the library that uses it.

Hope that makes sense, I can go either way with this.

Thanks,
Michael

from bolt_sips.

That's what I was doing, but then I realized the ack_failure has no meaningful role, from the user' perspective, and it's just an extra call that maybe was better if absorbed by Boltex internally, as it is anyway part of the Bolt workflow?! Still not sure, that's why I was asking. Trying to clarify (to myself) where the boundaries between Boltex and a client implementation will be? For example: Is Boltex supposed to returning :ok/:error and that's all, or is it the Client needing to call Boltex in case of :error .... to reset the server session? The latter shouldn't be the responsibility of the client code, since Neo4j may change this behavior, hence absorbed at the lowest level? Again, just thinking out loud. I can definitely call ack_failure at my end :) Thank you for your input!

from bolt_sips.

I'll call ack_failure at my end for now, until we figure out a better approach :)

from bolt_sips.

@mschae - can you please apply this patch to the add-dbconnection branch? It contains a PR from @wli0503; details in the comments below. Thank you.

struct_handler_for_encoder.patch.zip

BOLT: Struct Handler for PackEncoder (@wli0503)

- Add a `@fallback_to_any` directive for `Boltex.PackStream.Encoder`
  for falling back to `Any` implementation
- Add a new implementation for `Any`
  * Specifically, when the passed structure is a struct, it will
    fall into the category since `is_map/1` guard clause will work
  * It transform the `struct` to `map` and then calling
    `Boltex.PackStream.Encoder.encode/1`, which will invoke the
    implementation for the `Map` encoder
- Add two test cases in `query_test.exs` to test the code handles
  `Map` and `Struct` correctly
  
https://github.com/florinpatrascu/bolt_sips/pull/14

from bolt_sips.

Fixed. I took the liberty to add a catch all that raises a Boltex.PackStream.EncodeError when trying to encode something we can't encode (for example tuples).

from bolt_sips.

Awesome! Thanks Michael! Unfortunately I can't try it out until later on, but I'll get to it in the afternoon, can't wait till I get home :) All my tests, especially the ones involving transactions, were green last night so I am pretty sure we're good to go?! Michael, if Boltex is in a shape it's also suitable to your other project db-conn... then maybe is worth trying to push it to hex.pm?

from bolt_sips.

Hi Michael - had to debug some failing tests and found the culprit. Please apply this patch too:

diff --git a/lib/boltex/error.ex b/lib/boltex/error.ex
index e7a109a..4deb72e 100644
--- a/lib/boltex/error.ex
+++ b/lib/boltex/error.ex
@@ -69,6 +69,7 @@ defmodule Boltex.Error do
   end
 
   defp get_id(nil), do: nil
+  defp get_id({:sslsocket, {:gen_tcp, port, _tls, _unused_yet}, _pid} = p), do: get_id(port)
   defp get_id(port) do
     case Port.info(port, :id) do
       {:id, id} -> id

The Error.get_id/1, didn't know about :sslsocket. I wasn't sure about your plans regarding this function, we can make it more verbose later, but for now I left out these: _tls, _unused_yet and _pid.

I tested with both standard :ssl and :etls, and Boltex is now working with either, which means I can go to bed :) 🍻

from bolt_sips.

Hey,

I pushed that fix up to the branch. I'll go ahead and push v0.2.0 as soon as you give me the green light.

Thanks!

from bolt_sips.

HI there - it works, thank you! You can release the Kraken anytime it's convenient for you :) On my side, later on, I'll rework a bit the code to deal with the new encoding exception and will also push the newer version to hex.pm, using: Boltex, this time. Thanks again, Michael!

from bolt_sips.

Awesome thanks! So glad this is now consolidated. If you find any issues with Boltex feel free to add a PR!

I released v0.2.0. Should be all there!

❤️

from bolt_sips.

Thank you, you too Michael. Great work! I'll push my changes as well, later on, and will publish the new version at hex.pm. I'll send you any PRs from now on, if need be.

Awesome, indeed!
❤️

Edit:

• can you publish the new version to Hex? Unless it is there already, but I cannot see it yet.
  NVM I can see it, thank yoU!

from bolt_sips.

Package published to https://hex.pm/packages/bolt_sips/0.2.0

This is good stuff, having the two projects consolidated! Thanks again, Michael!

🍻

from bolt_sips.