search

florinpatrascu / bolt_sips

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

feat: support connection options in queries #82

Closed tcrossland closed 4 years ago

tcrossland commented 4 years ago

I needed to support a longer timeout than the default (15 seconds) for a bulk query... this pull request adds support for passing options through to DBConnection.execute/4.

sourcelevel-bot[bot] commented 4 years ago

Hello, @tcrossland! This is your first Pull Request that will be reviewed by SourceLevel, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

florinpatrascu commented 4 years ago

Hi @tcrossland - thank you for the PR. I wonder tho', is the intent here to pass custom DBConnection options, per query? This would not work for a DBConnection pool that's already started. Also, you can define any configurable parameters that DBConnection knows, via the driver config, i.e.

config :bolt_sips, Bolt,
  url: "bolt://localhost",
  basic_auth: [username: "neo4j", password: "test"],
  pool_size: 10,
  max_overflow: 2,
  queue_interval: 500,
  queue_target: 1500

pls observe: :queue_interval and the :queue_target.

tcrossland commented 4 years ago

Hi @florinpatrascu, the options are passed through to DBConnection.execute, where a timeout can be specified. The default value of 15_000 was insufficient for certain bulk queries.

florinpatrascu commented 4 years ago

I was trying to clarify, that the "deafult" of 15secs that you are referring to, can be overridden by the driver's config. Example: {:ok, _neo} = Sips.start_link(url: "bolt://neo4j:test@localhost", timeout: 5_000). This later will become the DBConnection :timeout value, that will be available to any subsequent calls. Ecto.Repo works the same.

But there is value in the PR too, should we later want to add custom configurations. So I'm ok with merging it 👍

@dominique-vassard and @kristofka, what's your thoughts on this?

kristofka commented 4 years ago

I think it makes sense to allow passing options to DbConnection.execute/4. Postgrex also allows it ( see https://hexdocs.pm/postgrex/Postgrex.html#execute/4 ). The allowed options should probably be documented though.

dominique-vassard commented 4 years ago

I'm ok with this update. Regarding bolt_sips.ex, I think that just adding opts \\ [] to the currently existing query and query! functions is enough to do the job. Thanks!

tcrossland commented 4 years ago

@florinpatrascu thanks and apologies for delayed response... I’m pretty sure I tried using the timeout in the driver config but still had timeout issues from DBConnnection. But that may have been previous to 2.0. Anyway, passing the option through on the specific bulk query is useful at least for our use case.

florinpatrascu commented 4 years ago

No worries! The change is in the latest distribution, available from hex.pm. And thanks again for your PR ❤️