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

feat: add timeout for query #539

Merged
merged 8 commits into from
Jan 10, 2023
Merged

Conversation

jiacai2050
Copy link
Contributor

@jiacai2050 jiacai2050 commented Jan 5, 2023

Which issue does this PR close?

Closes #165

Rationale for this change

See issue above

This PR only support set timeout in config file, will support set timeout in request level in next PR.

What changes are included in this PR?

In principle, if all operations are async/await, we can just timeout at the beginning of query, but it's not that ideal in real case, there maybe exists

So we have to add timeout detect in those cases beside top-level timeout. AKAIK, there are two places need special handling:

  • Memtable scan, include sync operation
  • SST scan, include spawned tasks

When implement this feature, I also find TableProvider is wrongly cached, please see comments in

Are there any user-facing changes?

No

How does this change test

Existing UT, and manually test a large table with select count(1) from cpu;, it will return following error after 5s timeout

$ time curl --location --request POST 'http://0:5446/sql' --data-raw 'select count(1) from cpu;'

{"code":500,"message":"Failed to handle request, err:Query timeout, query:select count(1) from cpu;, err:deadline has elapsed"}
real    0m5.013s
user    0m0.008s
sys     0m0.000s

@jiacai2050 jiacai2050 changed the title feat add timeout for query feat: add timeout for query Jan 6, 2023
@jiacai2050 jiacai2050 marked this pull request as ready for review January 6, 2023 09:16
analytic_engine/src/memtable/skiplist/iter.rs Outdated Show resolved Hide resolved
interpreters/src/context.rs Outdated Show resolved Hide resolved
proto/protos/remote_engine.proto Outdated Show resolved Hide resolved
server/src/mysql/service.rs Outdated Show resolved Hide resolved
sql/src/provider.rs Show resolved Hide resolved
table_engine/src/table.rs Outdated Show resolved Hide resolved
proto/protos/remote_engine.proto Outdated Show resolved Hide resolved
analytic_engine/src/memtable/skiplist/iter.rs Outdated Show resolved Hide resolved
Copy link
Member

@ShiKaiWi ShiKaiWi left a comment

Choose a reason for hiding this comment

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

LGTM

@jiacai2050 jiacai2050 merged commit f9bbaad into apache:main Jan 10, 2023
@jiacai2050 jiacai2050 deleted the feat-timeout branch January 10, 2023 10:00
chunshao90 pushed a commit to chunshao90/ceresdb that referenced this pull request May 15, 2023
* add timeout when collect

* add timeout control for memtable scan and sst scan

* add timeout for user-facing service, like grpc, http

* add timeout in top level execute

* remove deadline from sst options

* timeout http handle at top level

* refactor timeout to Option

* add check_deadline in InstantExt
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.

Add timeout parameter to request
2 participants