search

scylladb / gocql

Package gocql implements a fast and robust ScyllaDB client for the Go programming language.
https://docs.scylladb.com/stable/using-scylla/drivers/cql-drivers/scylla-go-driver.html
BSD 3-Clause "New" or "Revised" License
169 stars 47 forks source link

Do not choose less busy connection if query is LWT #208

Closed sylwiaszunejko closed 1 week ago

sylwiaszunejko commented 1 week ago

There is a code in gocql that selects another connection to a node if chosen connection is too busy (has less than half available stream ids).

We should not do this with LWT.

sylwiaszunejko commented 1 week ago

I thought it might be better to have Pick(Token, ExecutableQuery) *Conn interface instead of Pick(Token, string, string, bool) *Conn but I had issues with rewriting scylla_test.go with that version so I decided to first go with the simpler solution and ask about second opinion.

Lorak-mmk commented 1 week ago

Out of all the LWT fixes this one is the least urgent, imo we can do it properly. What are the problems that you faced?

sylwiaszunejko commented 1 week ago

Out of all the LWT fixes this one is the least urgent, imo we can do it properly. What are the problems that you faced?

I just wasn't sure how to mock ExecutableQuery for the test purpose, but I guess I could just implement something like MockQuery and implement all of the methods as empty (returning nil, "", false etc.) to have .Keyspace(), .Table() and IsLWT() for the purpose of Pick (e.g. here: https://github.com/scylladb/gocql/blob/master/scylla_test.go#L27)

Lorak-mmk commented 1 week ago

One thing to consider; is type ConnPicker interface something that the user may implement? Because this PR changes it's definition, so if it is exposed to the user (I'm not sure how that works in Go) then this is a breaking change

dkropachev commented 1 week ago

There is a code in gocql that selects another connection to a node if chosen connection is too busy (has less than half available stream ids).

We should not do this with LWT.

@sylwiaszunejko , why we should not do that with LTW queries ? is there core issue on that or something that have details on reasoning ?

dkropachev commented 1 week ago

There is a code in gocql that selects another connection to a node if chosen connection is too busy (has less than half available stream ids). We should not do this with LWT.

@sylwiaszunejko , why we should not do that with LTW queries ? is there core issue on that or something that have details on reasoning ?

I think I found answer, please correct me if I am wrong, but related issues are: https://github.com/scylladb/scylla-enterprise/issues/4369 https://github.com/scylladb/gocql/issues/201

dkropachev commented 1 week ago

Out of all the LWT fixes this one is the least urgent, imo we can do it properly. What are the problems that you faced?

I just wasn't sure how to mock ExecutableQuery for the test purpose, but I guess I could just implement something like MockQuery and implement all of the methods as empty (returning nil, "", false etc.) to have .Keyspace(), .Table() and IsLWT() for the purpose of Pick (e.g. here: https://github.com/scylladb/gocql/blob/master/scylla_test.go#L27)

I guess that in most cases you can just pass nil

dkropachev commented 1 week ago

One thing to consider; is type ConnPicker interface something that the user may implement? Because this PR changes it's definition, so if it is exposed to the user (I'm not sure how that works in Go) then this is a breaking change

Greate idea, but it would be better to address it in separate PR since not only connpicker is currently not a publicly availalbe, but also we will have to somehow provide API that would suggest to use certain conn picker in certain conditions, or, maybe have one connpicker to handle all the cases.

dkropachev commented 1 week ago

@sylwiaszunejko, I made attempt to make Pick(Token, ExecutableQuery), please take a look.

Lorak-mmk commented 1 week ago

One thing to consider; is type ConnPicker interface something that the user may implement? Because this PR changes it's definition, so if it is exposed to the user (I'm not sure how that works in Go) then this is a breaking change

Greate idea, but it would be better to address it in separate PR since not only connpicker is currently not a publicly availalbe, but also we will have to somehow provide API that would suggest to use certain conn picker in certain conditions, or, maybe have one connpicker to handle all the cases.

I wasnt suggesting to make it publicly available. If it isn't - great, we don't have to worry about breaking changes.

sylwiaszunejko commented 1 week ago

@dkropachev I used the code proposed by you just changed two lines to simplify