Skip to content

CASSGO-61 fix connection timeout override with custom #1866

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

CASSGO-61 fix connection timeout override with custom #1866

wants to merge 1 commit into from

Conversation

@tengu-alt
Copy link
Contributor

@tengu-alt tengu-alt commented Feb 27, 2025 •
edited
Loading

Resolves #953
This PR provides the fix for the connection timeout override via query context timeout, allowing users to set the custom timeout for the query.

@tengu-alt tengu-alt changed the title Implement connection timeout override with custom CASSGO-61-Implement connection timeout override with custom Feb 27, 2025
@tengu-alt tengu-alt force-pushed the conn-exec-override-timeout branch from 2704a12 to deb64e8 Compare February 27, 2025 14:55
@tengu-alt tengu-alt changed the title CASSGO-61-Implement connection timeout override with custom CASSGO-61-fix connection timeout override with custom Feb 27, 2025
@tengu-alt tengu-alt force-pushed the conn-exec-override-timeout branch 2 times, most recently from 951c58d to ae2efae Compare March 3, 2025 11:09
@tengu-alt tengu-alt force-pushed the conn-exec-override-timeout branch from ae2efae to 73bf8bb Compare March 21, 2025 12:54
OleksiienkoMykyta
OleksiienkoMykyta reviewed Mar 24, 2025
View reviewed changes
Copy link
Contributor

@OleksiienkoMykyta OleksiienkoMykyta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it would be nice to provide details to documentation with code examples, how to set a custom timeout, and how it works.

In general, LGTM)

cassandra_test.go Outdated
}
session.executor.pool.mu.Unlock()
err = session.Query("INSERT INTO gocql_test.named_query(id, value) VALUES(2, 'value')").Exec()
if err == nil || err != ErrTimeoutNoResponse {
Copy link
Contributor

@OleksiienkoMykyta OleksiienkoMykyta Mar 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are expecting exactly ErrTimeoutNoResponse, this part is redundant "err == nil" wdyt?

Copy link
Contributor Author

@tengu-alt tengu-alt Mar 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, fixed.

@tengu-alt tengu-alt force-pushed the conn-exec-override-timeout branch from 73bf8bb to 1d3d896 Compare March 24, 2025 11:56
@tengu-alt
Copy link
Contributor Author

tengu-alt commented Mar 24, 2025

Also, it would be nice to provide details to documentation

I've added the code example to the query.WithContext() method.

@tengu-alt tengu-alt force-pushed the conn-exec-override-timeout branch from 1d3d896 to 455a01c Compare March 24, 2025 12:10
@joao-r-reis joao-r-reis changed the title CASSGO-61-fix connection timeout override with custom CASSGO-61 fix connection timeout override with custom Mar 25, 2025
@jameshartig
Copy link
Contributor

jameshartig commented Mar 31, 2025

When the context is cancelled because it had a deadline, we aren't calling c.handleTimeout. We probably need to check the context error and if it had a deadline when <-ctxDone and then call c.handleTimeout.

@tengu-alt tengu-alt force-pushed the conn-exec-override-timeout branch from 455a01c to 62d08a2 Compare April 1, 2025 11:55
@tengu-alt
Copy link
Contributor Author

tengu-alt commented Apr 1, 2025

We probably need to check the context error and if it had a deadline when <-ctxDone and then call c.handleTimeout.

Agree. I've also added this check here in case if deadline hits before we create ctxDone chan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@OleksiienkoMykyta OleksiienkoMykyta OleksiienkoMykyta approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Session Timeout and Context Timeout

3 participants

@tengu-alt @jameshartig @OleksiienkoMykyta