fix: return query if it already exists #15207
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eschutho
commented
Jun 17, 2021
| client_id=client_id, user_id=g.user.get_id(), sql_editor_id=tab_state_id | ||
| client_id=client_id, | ||
| user_id=g.user.get_id(), | ||
| sql_editor_id=str(tab_state_id), |
There was a problem hiding this comment.
I was getting an error when trying to delete the query b/c it was passing in an int here.
eschutho
force-pushed
the
elizabeth/fix-db-timeout-error
branch
from
567d6b6
to
65aa038
Compare
betodealmeida
approved these changes
Jun 17, 2021
Codecov Report
@@ Coverage Diff @@
## master #15207 +/- ##
==========================================
- Coverage 77.09% 76.93% -0.16%
==========================================
Files 971 971
Lines 50236 50242 +6
Branches 6494 6494
==========================================
- Hits 38729 38656 -73
- Misses 11302 11381 +79
Partials 205 205
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
/testenv up |
|
@eschutho Ephemeral environment spinning up at http://50.112.146.179:8080. Credentials are |
|
did a visual test on ephemeral env and it looked good. |
|
Ephemeral environment shutdown and build artifacts deleted. |
|
🏷️ 2021.25 |
henryyeh
pushed a commit
to preset-io/superset
that referenced
this pull request
Jun 23, 2021
* check if query exists before saving a new one * fix test (cherry picked from commit 58cc78d)
henryyeh
pushed a commit
to preset-io/superset
that referenced
this pull request
Jun 23, 2021
* check if query exists before saving a new one * fix test (cherry picked from commit 58cc78d)
cccs-RyanS
pushed a commit
to CybercentreCanada/superset
that referenced
this pull request
Dec 17, 2021
* check if query exists before saving a new one * fix test
QAlexBall
pushed a commit
to QAlexBall/superset
that referenced
this pull request
Dec 29, 2021
* check if query exists before saving a new one * fix test
cccs-rc
pushed a commit
to CybercentreCanada/superset
that referenced
this pull request
Mar 6, 2024
* check if query exists before saving a new one * fix test
mistercrunch
added
🏷️ bot
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
We were seeing some duplicate client_id errors when running queries. Here's the root cause:
When we have a timeout on a query, we return a 408 , and when most browsers get a 408 response, they automatically retry the request. The issue with the query POST request is that the db request happens after we create the query, so if the db request times out, then the retry fails because the request is not idempotent, due to the fact that the client_id is created on the client and has a uniqueness constraint in the db.
My solution here is to check to see if the query exists and if it is in a pending or timed out state, don't resave it, but rather just return the original query.
I had thought of a few different options for this issue, and here are the reasonings why I didn't go with other solutions:
Some concerns/questions:
If a query is new, and happens to match an existing query that is running, (unlikely but possible), we don't want to return someone else's query. There's a check on this lookup for user_id, but that can be null. There's also a lookup on sql_editor_id. My assumption is that with the combination of these three factors, we should with certainty only get the request that we are intending, but I'd welcome another set of eyes on that. The last option is to check the sql string, but the comparison on what could potentially be a long string may be costly.
In my testing, I was never able to reproduce a server timeout response, only a netstat error, and the query continued to have a status of "running" which means that it will continue to be polled for. You have to explicitly delete the query if you navigate away from the page without stopping the query. I didn't change that behavior, but it's something that we may want to investigate.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No visual changes
TESTING INSTRUCTIONS
If you are testing locally and can put a debug statement somewhere in the sql_json POST request you can replicate a timeout. After about 2 minutes, you should see another sql_json POST request in your browser network tab, and it should return a 200 with the original query, and not raise an error.
ADDITIONAL INFORMATION