search

whatyouhide / xandra

Fast, simple, and robust Cassandra/ScyllaDB driver for Elixir.
ISC License
406 stars 54 forks source link

Changelog for 0.16.0 is missing change to transport_options #313

Closed relistan closed 1 year ago

relistan commented 1 year ago

In 0.16.0 the config key transport_options is no longer a top level configuration key. It now must be passed as connection_options: [ transport_options: ... ] and it silently ignores transport_options if it was passed. If you upgraded from pre-0.16.0 and had transport_options configured, they are now ignored. This change is not called out anywhere I could find and took quite some digging to figure out.

whatyouhide commented 1 year ago

Hey Karl!! Sorry for the trouble here, yeah 😩

First things first, you're talking about Xandra.Cluster, right?

How were you calling this before? It was an unintentional change, so I can either revert it or make it warn (to avoid the silent failure, which sucks!)

relistan commented 1 year ago

@whatyouhide, no worries, we all owe you for continuing to maintain this excellent driver. I opened an issue rather than a PR because I wasn't sure the right fix. It kinda needs to go into the release notes for a tag that already shipped. We are OK with either leaving it how it is in 0.16.0 or rolling back to the previous key structure.

whatyouhide commented 1 year ago

@relistan so, I tried to reproduce this. If I start a cluster as:

Xandra.Cluster.start_link(
  transport_options: [fd: 10]
)

then I get the right :transport_options in the control connection as well as in each Xandra. To attest to this, the connection fails (because 10 is not a valid file descriptor here).

If, instead, I go with :connection_options, then I don't get errors since it's sweeped under the rug:

Xandra.Cluster.start_link(
  connection_options: [transport_options: [fd: 10]]
)
relistan commented 1 year ago

Spending a little more time on this, I think it is passing the configuration. There is a Keyword.split/2 call in Cluster.start_link/1 which seems to prune off settings that are not in the NimbleOptions config and passes them on down to the control connection. So, apologies for the wasted time. The logic of the module is quite different and I started from the bottom (ControlConnection) rather than the top, Cluster. It seems the issues we are experiencing are something else.

whatyouhide commented 1 year ago

@relistan don't apologize!! Yeah, that's the same route I went through and I got to the same conclusion. I opened https://github.com/elixir-ecto/db_connection/pull/290 and https://github.com/elixir-ecto/db_connection/pull/291 to expose the options that end up in DBConnection, that way I can raise if any options are not for Xandra, the cluster, or DBConnection... this should help 😄