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.

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

Add timeout option while running queries over HTTP #3238

Merged
merged 1 commit into from
Apr 2, 2019
Merged

Add timeout option while running queries over HTTP #3238

merged 1 commit into from
Apr 2, 2019

Conversation

mangalaman93
Copy link
Contributor

@mangalaman93 mangalaman93 commented Apr 1, 2019 •
edited
Loading

The endpoint /query now accepts a timeout key - /query?timeout=100ms.

This change is Reviewable

@danielmai danielmai changed the title Add timeout option while running queries Add timeout option while running queries over HTTP Apr 1, 2019
@danielmai
Copy link
Contributor

Can you make the timeout accept a duration instead of just milliseconds? i.e., ?timeout=1s or ?timeout=1m for 1 second or 1 minute.

danielmai
danielmai reviewed Apr 1, 2019
View reviewed changes
Copy link
Contributor

@danielmai danielmai left a comment

Choose a reason for hiding this comment

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

The test failure you mentioned didn't happen in CI.

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @codexnull, @paulftw, and @srfrog)

mangalaman93
mangalaman93 commented Apr 1, 2019
View reviewed changes
Copy link
Contributor Author

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

Can you make the timeout accept a duration instead of just milliseconds? i.e., ?timeout=1s or ?timeout=1m for 1 second or 1 minute.

Any ideas of a standards and a parser around this? time package does this?

The test failure you mentioned didn't happen in CI.

May be it happens when you run it twice. Was able to reproduce it on desktop.

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @codexnull, @paulftw, and @srfrog)

@martinmr
Copy link
Contributor

martinmr commented Apr 1, 2019

https://golang.org/pkg/time/#ParseDuration should work.

@srfrog
Copy link
Contributor

srfrog commented Apr 1, 2019

I find context.WithTimeout a bit easier to use since you only need to send the actual timeout. So it fits perfectly with time.ParseDuration

@mangalaman93
Add timeout option while running queries
af9a8b9 
The endpoint /query now accepts a timeout key /query?timeout=100ms
If the query doesn't finish within the timeout time, query is
stopped and an error is returned.
@mangalaman93 mangalaman93 marked this pull request as ready for review April 2, 2019 04:23
manishrjain
manishrjain approved these changes Apr 2, 2019
View reviewed changes
Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @codexnull, @paulftw, and @srfrog)

@mangalaman93 mangalaman93 merged commit 913c920 into master Apr 2, 2019
@mangalaman93 mangalaman93 deleted the aman/timeout branch April 2, 2019 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielmai danielmai danielmai left review comments

@manishrjain manishrjain manishrjain approved these changes

@srfrog srfrog Awaiting requested review from srfrog

@codexnull codexnull Awaiting requested review from codexnull

@paulftw paulftw Awaiting requested review from paulftw

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mangalaman93 @danielmai @martinmr @srfrog @manishrjain