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

Use `newFramerWithExts` instead of `newFramer` to utilize protocol extensions #173

Closed sylwiaszunejko closed 2 months ago

sylwiaszunejko commented 2 months ago

When LWT optimization was implemented in gocql (https://github.com/scylladb/gocql/pull/49), all newFramer calls were replaced with newFramerWithExts calls.

Previously, during the upstream merging the code using newFramer was added (https://github.com/scylladb/gocql/pull/109) and it was forgotten to replace it with newFramerWithExts. That way, the flagLWT field was set to 0 by default, causing the driver to consider every query executed as a LWT query.

The issue was not immediately noticed because the only difference in behavior occurs when we want to use ShuffleReplicas() in TokenAwareHostPolicy.

This PR fixes the issue by replacing newFramer with newFramerWithExts.

Fixes: #174

avelanarius commented 2 months ago

One small nit, a couple suggestions regarding the commit description:

Use newFramerWithExts instead of newFramer to utilize protocol extensions

The commit title is a bit too long (for example longer than 50/72 rule), but I admit that it's hard to word it better due to long newFramerWithExts name.

In order to utilize cql protocol extensions, we need to use newFramerWithExts instead of newFramer.

Suggestion: When LWT optimization was implemented in gocql (PR number), all newFramer calls were replaced with newFramerWithExts calls.

Previously, during the upstream merging the code using newFramer was added (https://github.com/scylladb/gocql/pull/)109. That way, the flagLWT field was set to 0 by default, causing the driver to consider every query executed as a LWT query.

Nit: ) should be placed after 109 so that the link is rendered correctly (already fixed it in PR description myself). You could mention specifically that we forgot to replace newFramer with newFramerWithExts in that upstream merging.

The issue was not immediately noticed because the only difference in behavior occurs when we want to use ShuffleReplicas() in TokenAwareHostPolicy.

This commit fixes that.

Nit: it's not immediately clear that "that" refers to newFramerWithExts not ShuffleReplicas() mentioned in the previous sentence.

roydahan commented 2 months ago

Maybe it would be good to open an issue for it and set this PR as fixing it. What's the impact on the affected versions? (performance impact?) What are the affected versions? (relevant if there is an actual impact).

sylwiaszunejko commented 2 months ago

@avelanarius I made the changes to the commit and PR description. I am not sure how to change the title to be short but meaningful, do you have any suggestions or should I leave it as it is?

sylwiaszunejko commented 2 months ago

Maybe it would be good to open an issue for it and set this PR as fixing it. What's the impact on the affected versions? (performance impact?) What are the affected versions? (relevant if there is an actual impact).

@roydahan I have created an issue.

sylwiaszunejko commented 2 months ago

@avelanarius Should I make some more changes to this PR or can we merge it as it is?

mykaul commented 1 month ago

@sylwiaszunejko - can you quantify the performance impact? Is it something we've measured?

sylwiaszunejko commented 1 month ago

@sylwiaszunejko - can you quantify the performance impact? Is it something we've measured?

@mykaul We do not have any measure for the performance impact. As stated in the PR description, the only difference in behavior occurs when we use ShuffleReplicas() in TokenAwareHostPolicy and the query is not LWT. I am not sure how often these kinds of scenarios happen.