Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use newFramerWithExts instead of newFramer to utilize protocol extensions #173

Merged
merged 1 commit into from
May 15, 2024

Conversation

sylwiaszunejko
Copy link
Collaborator

@sylwiaszunejko sylwiaszunejko commented May 9, 2024

When LWT optimization was implemented in gocql (#49), all newFramer calls were replaced with newFramerWithExts calls.

Previously, during the upstream merging the code using newFramer was added (#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

@sylwiaszunejko sylwiaszunejko self-assigned this May 9, 2024
@avelanarius avelanarius requested a review from Lorak-mmk May 9, 2024 14:41
@avelanarius
Copy link

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
Copy link
Collaborator

roydahan commented May 9, 2024

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
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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.

…ions

When LWT optimization was implemented in gocql
(scylladb#49), all `newFramer`
calls were replaced with `newFramerWithExts` calls.

Previously, during the upstream merging the code using `newFramer`
was added (scylladb#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 commit fixes the issue by replacing `newFramer` with `newFramerWithExts`.

Fixes: scylladb#174
@sylwiaszunejko
Copy link
Collaborator Author

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

@avelanarius avelanarius merged commit 7f9ad1c into scylladb:master May 15, 2024
1 check passed
@mykaul
Copy link

mykaul commented May 30, 2024

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

@sylwiaszunejko
Copy link
Collaborator Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Queries are marked as LWT incorrectly
4 participants