Conversation
baibhavbista
left a comment
baibhavbista
left a comment
There was a problem hiding this comment.
added some clarifying comments
|
|
||
| ### Testing | ||
|
|
||
| We have scripts in the `script` folder like `./script/test_clj.sh` |
There was a problem hiding this comment.
added these to the docs because I couldn't get the below steps (the ones with kaocha) working
| (defn collect [context symbols] | ||
| (into #{} (map vec) (-collect context symbols))) | ||
| (into #{} | ||
| (map #(do (timeout/assert-time-left) |
There was a problem hiding this comment.
change additional to datalevin timeout. Realized it was required here because this is the place where laziness ends
| resolved | ||
| tuple)) | ||
| ;; realize lazy seq because this is the last step anyways, and because if we don't realize right now then binding for timeout/*deadline* does not work | ||
| doall))) |
There was a problem hiding this comment.
another change which is not present in datalevin timeout PR. This is needed otherwise pull within find spec of q do not obey timeout
| true | ||
| (-post-process find (:qreturn-map parsed-q))))) | ||
| (let [parsed-q (lru/-get *query-cache* q #(dp/parse-query q))] | ||
| (binding [timeout/*deadline* (timeout/to-deadline (:qtimeout parsed-q))] |
There was a problem hiding this comment.
main change here is just the addition of the binding in the middle of the let
| @@ -0,0 +1,24 @@ | |||
| (ns ^:no-doc datascript.timeout) | |||
There was a problem hiding this comment.
| @@ -0,0 +1,51 @@ | |||
| (ns datascript.test.query-deadline | |||
There was a problem hiding this comment.
tests ns, also copied from datalevin . Source : https://github.com/juji-io/datalevin/blob/master/test/datalevin/test/query_deadline.clj
|
Interesting! Can you talk a little when is this useful (I am not saying that it isn’t, just want to understand use-cases) Implementation-wise:
|
|
Sorry just seeing your reply now @tonsky ! The main usecase is when allowing end users to be able to run their own queries. Queries not properly written can cause combinatorial explosion of candidates, and lead either to OOM or to the application freezing for a non-allowably-long period of time (common use case of datascript running in the main thread in browser). In these cases, a timeout allows one to stop the query partway through if the time taken exceeds a value we deem reasonable. Re: your implementation notes, I will go through them properly next week. BTW, as I mentioned in the PR description, most of your "why"s could be answered by "it was done that way in datalevin, and the relevant parts of the two codebases looked similar enough that I just recreated the same changes here in datascript". I will more properly think about your notes and reply/fix soon though |
2 participants
Adds a timeout to
qandpullMostly inspired from the timeout approach in datalevin: https://github.com/juji-io/datalevin/blob/master/src/datalevin/timeout.clj (& original PR which adds it)
Is this approach correct, @tonsky ? (seems to work, based on my tests)