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

[QUESTION] concurrent query failed on cancelled requests #372

Open
tommsawyer opened this issue Oct 26, 2023 · 2 comments
Open

[QUESTION] concurrent query failed on cancelled requests #372

tommsawyer opened this issue Oct 26, 2023 · 2 comments

Comments

@tommsawyer
Copy link

Which feature your question relates to?
concurrent requests

Question details
When two identical requests are sent to the CHproxy, it only executes the first one, while the second one waits and sends the same result. If the user cancels the first request, the second one is also canceled with the error "concurrent query failed". Is this expected behavior? It seems to me that in this case the second request should be launched and return the correct result from the clickhouse.

Additional context/motivations
We connected CHProxy to Grafana, and Grafana has a refresh button to reload the data. But sometimes it tries to update itself based on a timer or a html element blur event. In this case, two requests are sent simultaneously, and grafana cancels the first one. CHproxy cancels the second one, so no data is reloaded.

@mga-chka
Copy link
Collaborator

Hi,

The behavior of the "concurrent queries" feature is the one you mentioned: if a query fails, the ones that were run at the same time will fail: https://github.com/ContentSquare/chproxy/blob/master/proxy.go#L396

Thanks for the additional context, we were not aware Grafana was behaving this way.
If the error in case of cancelation is specific, we could have a dedicated behavior for this error.
The core maintainers won't have time in the next months to look at it, so if you have time, feel free to create a PR and we will review it.

@Blokje5
Copy link
Collaborator

Blokje5 commented Dec 12, 2023

I took some time to look into it: my gut feeling is that we need to add an exception to the recoverable status codes for 499 (the status we return from the reverse proxy on cancel). Or maybe we should add the canceled

However, I wasn't able to reproduce it in a test yet. I don't want to create a PR without being able to reproduce it in a test. I'll probably come back to this after the holidays though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants