Clean up timeouts in the conn and add :max_concurrent_requests_per_connection
#358
Conversation
| @@ -392,6 +392,20 @@ defmodule Xandra do | |||
| *Available since v0.18.0*. | |||
| """ | |||
| ], | |||
| max_concurrent_requests_per_connection: [ | |||
There was a problem hiding this comment.
This is here so that we can test it.
| # Reset in-flight requests and timed out stream IDs. We just disconnected, so all the | ||
| # in-flight requests (including the timed-out ones) are now invalid and the server should | ||
| # have killed them anyway. | ||
| data = %__MODULE__{data | in_flight_requests: %{}, timed_out_ids: %{}} |
There was a problem hiding this comment.
If we do this, we don't have to clean things up when disconnected.
| {nil, data} when is_map_key(timed_out_ids, stream_id) -> | ||
| :telemetry.execute( | ||
| [:xandra, :debug, :received_timed_out_response], | ||
| %{}, | ||
| telemetry_meta(data, %{stream_id: stream_id}) | ||
| ) |
There was a problem hiding this comment.
Refactored to flatten the nesting.
| @@ -265,8 +263,7 @@ defmodule Xandra.Connection do | |||
| {:error, {:connection_crashed, reason}} | |||
| after | |||
| timeout -> | |||
| :telemetry.execute([:xandra, :client_timeout], %{}, telemetry_metadata) | |||
There was a problem hiding this comment.
We don't need this, clients can measure this themselves for now if they want. I want to first run this code in your codebase and see how things go before making this a public Telemetry event.
There was a problem hiding this comment.
Sure, I can test this. And yeah, it is measurable from the client ofc. but I thought that it might be an interesting metric.
There was a problem hiding this comment.
@harunzengin it probably will be in the future, but I want to avoid committing to an API for the telemetry event until we're sure.
| # for it that timed out on the caller's side. Let's just emit a | ||
| {nil, data} when is_map_key(timed_out_ids, stream_id) -> | ||
| :telemetry.execute( | ||
| [:xandra, :debug, :received_timed_out_response], |
There was a problem hiding this comment.
For now, let's keep this a "debug" event so that you folks can measure this but we don't have to make it part of the public API.
| @@ -265,8 +263,7 @@ defmodule Xandra.Connection do | |||
| {:error, {:connection_crashed, reason}} | |||
| after | |||
| timeout -> | |||
| :telemetry.execute([:xandra, :client_timeout], %{}, telemetry_metadata) | |||
There was a problem hiding this comment.
Sure, I can test this. And yeah, it is measurable from the client ofc. but I thought that it might be an interesting metric.
| end | ||
| end) | ||
| ) | ||
| new_timed_out_ids = |
There was a problem hiding this comment.
With 4e2ac5e, I get the following stacktrace, just trying to execute a query:
** (exit) exited in: :gen_statem.call(#PID<0.933.0>, {:checkout_state_for_next_request, #Reference<0.0.121219.3000101197.1395982337.31962>}, :infinity)
** (EXIT) an exception was raised:
** (BadMapError) expected a map, got: []
(erts 14.2.1) :erlang.is_map_key(21374, [])
(xandra 0.18.1) lib/xandra/connection.ex:900: Xandra.Connection.random_free_stream_id/2
(xandra 0.18.1) lib/xandra/connection.ex:600: Xandra.Connection.connected/3
(stdlib 5.2) gen_statem.erl:1395: :gen_statem.loop_state_callback/11
(stdlib 5.2) proc_lib.erl:241: :proc_lib.init_p_do_apply/3
(stdlib 5.2) gen.erl:246: :gen.do_call/4
(stdlib 5.2) gen_statem.erl:923: :gen_statem.call/3
(xandra 0.18.1) lib/xandra/connection.ex:158: Xandra.Connection.execute/4
(xandra 0.18.1) lib/xandra/retry_strategy.ex:309: Xandra.RetryStrategy.run_on_cluster/5
There was a problem hiding this comment.
Also, it seems that preparing a query doesn't work and fails with :timeout
There was a problem hiding this comment.
@harunzengin I fixed the bug and added a regression test for that, it wasn't the nicest test to add but I’m happy we have a test for this now 🙃
As for the prepare, I’m not sure what you mean. I have a test in there that exercises Xandra.prepare/3 and it's passing. Do you have an example of when preparing a query fails?
There was a problem hiding this comment.
I tested this out, the timeouts in prepared were probably related to the first case, it is resolved now.
|
@harunzengin and no memory leaks or anything like that, correct? |
|
@whatyouhide No, it runs since yesterday and no memory leaks. I guess we're ready for a patch release? |
|
Yep, right after I fix #359. |
cc @harunzengin