Enable a timeout for Queries #179
Conversation
src/engine/Operation.h
Outdated
| computeResult(newResult.get()); | ||
| // lambda that computes the result | ||
| auto c = [this, &newResult]() { this->computeResult(newResult.get()); }; | ||
| // deferred futures are never run in parallel |
There was a problem hiding this comment.
It's not deferred in the current version. I think c deserves a nicer name.
src/engine/Server.cpp
Outdated
| @@ -352,7 +357,7 @@ string Server::composeResponseJson(const ParsedQuery& query, | |||
| os << ",\n"; | |||
|
|
|||
| os << "\"time\": {\n" | |||
| << "\"total\": \"" << _requestProcessingTimer.usecs() / 1000.0 << "ms\",\n" | |||
| << "\"total\": \"" << _requestProcessingTimer.msecs() / 1000.0 << "ms\",\n" | |||
There was a problem hiding this comment.
This seems wrong, if Timer::msecs() gives you milliseconds you shouldn't have to divide by 1000.0 to get "ms"
src/global/Constants.h
Outdated
| @@ -71,3 +73,6 @@ static constexpr uint8_t NUM_COMPRESSION_PREFIXES = 127; | |||
| // compression has been applied to a word | |||
| static const uint8_t NO_PREFIX_CHAR = | |||
| MIN_COMPRESSION_PREFIX + NUM_COMPRESSION_PREFIXES; | |||
|
|
|||
| static constexpr auto DEFAULT_QUERY_COMPUTATIION_TIMEOUT = 30s; | |||
| static constexpr auto DEFAULT_STRING_TRANSLATION_TIMEOUT = 30s; | |||
There was a problem hiding this comment.
I think I would set both to 1 minute and I think it would make sense to be able to override them with a query parameter. It's not too rare that we use QLever to extract things like "all names of all entities" which is necessarily slow but works quite well at the moment.
src/util/Timer.h
Outdated
|
|
||
| // ___________________________________________________________________ | ||
| long msecs() { | ||
| return std::chrono::duration_cast<std::chrono::microseconds>(getTime()) |
There was a problem hiding this comment.
I think "msecs" should be milliseconds not microseconds.
| //! Time from last mark to last stop (initally zero) | ||
| off_t usecs_since_mark() const { return _usecs - _usecs_at_mark; } | ||
| // ___________________________________________________________________ | ||
| long usecs() { return getTime().count(); } |
|
The latest changes look great. What do you think about adding a query parameter which overrides the timeout setting? I think something like that would be needed in our setting where we sometimes just need to support very large queries. Of course such a parameter should only be available with a flag since this obviously enables DoS |
- Implemented RuntimeSettings for anything that is not necessarily a compile-time constant (e.g. Timeout limits) - Moved the ResultTable->finish() call out of the Operation->computeResult(). That way this function does never hold locks and thus can be savely interrupted. - Moved the call to computeResult into a deferred future that does only get a certain time limit. If it is not ready after this limit, we abort the computation. TODO: - Implement timeout also for the translation of strings to IDs - Shall we handle timeout and abort differently when it comes to caching? E.G. can we extend the limit by executing the query twice? - Add command line flags that allow specifying timeout limits.
- using std::chrono instead of gettimeofday
Small compilation fix, learned sth about switch statements. Fixed Bugs and included review changes from Niklas
TODO: check actual diff on github
|
DeepCode encountered a problem when analyzing this pull request. If you want to retry, create a comment on this pull request with content: "Retry Deepcode". |
|
Retry Deepcode |
|
DeepCode analyzed this pull request. |
This allows to properly traverse the QueryExecutionTree as a tree in the sense of Graph theory.
- This allows cheap and correct copies of these Contexts per Query in case we want to have different settings for them.
- I recently found out that timeouts cannot be done the way I initially intended. However this branch still contains useful stuff on which I will continue to work soon.
|
@joka921 do I remember correctly that this approach turned out not to be viable? |
First version of a timeout PR
Implemented RuntimeSettings for anything that is not necessarily a compile-time constant (e.g. Timeout limits)
Moved the ResultTable->finish() call out of the Operation->computeResult(). That way this function does never hold locks and thus can be savely interrupted.
Moved the call to computeResult into a deferred future that does only get a certain time limit. If it is not ready after this limit, we abort the computation.
TODO:
Implement timeout also for the translation of strings to IDs
Shall we handle timeout and abort differently when it comes to caching? E.G. can we extend the limit by executing the query twice?
Add command line flags that allow specifying timeout limits.