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

add defaultKillQueryDeadline #87

Merged

Conversation

vano144
Copy link
Collaborator

@vano144 vano144 commented Dec 3, 2019

This pr fixes next problem: when killQuery option is enabled and readTimeout is equal to zero, context will have 0 limit as timeout

@vano144 vano144 force-pushed the feature/add-kill-on-query-timeout-constant branch 4 times, most recently from 89a8f0b to e5bab72 Compare December 4, 2019 10:17
@vano144 vano144 force-pushed the feature/add-kill-on-query-timeout-constant branch from e5bab72 to 481774d Compare December 4, 2019 10:29
@vano144 vano144 force-pushed the feature/add-kill-on-query-timeout-constant branch from 481774d to 3839fc1 Compare December 4, 2019 10:44
conn.go Outdated
@@ -36,6 +36,8 @@ var (
errEmptyQueryID = errors.New("query id is empty")
)

var defaultKillQueryDeadline = time.Duration(time.Second)
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets rename to defaultKillQueryTimeout because its actually a timeout, not a deadline

conn.go Outdated
@@ -192,7 +196,11 @@ func (c *conn) killQuery(req *http.Request, args []driver.Value) error {
return errEmptyQueryID
}
query := fmt.Sprintf("KILL QUERY WHERE query_id='%s'", queryID)
ctx, cancelFunc := context.WithTimeout(context.Background(), c.transport.ResponseHeaderTimeout)
limit := c.killQueryTimeout
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets rename limit to timeout

@DoubleDi DoubleDi merged commit 7605268 into mailru:master Dec 4, 2019
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.

None yet

2 participants