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: shorter timeout on test_connection #18001

Merged
merged 5 commits into from
Jan 12, 2022

Conversation

betodealmeida
Copy link
Member

@betodealmeida betodealmeida commented Jan 11, 2022

SUMMARY

When we run test_connection when adding a database, the response might error only after 1-2 minutes, depending on the parameters. The error comes back only when the network connection from the driver times out, and because this requests runs in a separate thread and in a 3rd party library we can't adjust that timeout.

We have a @timeout context manager in our code base, but they don't work here because SQLAlchemy runs the ping command in a separate thread. To enforce a shorter timeout on ping I used the func_timeout library, which spins off and monitors its own thread where the target function runs.

I also improved the error message (see screenshots below), and added logic to disable the "test connection" button while the test is in flight. In my initial attempt I added a spinner, but because the main thread is occupied while the test is in flight the spinner doesn't spin.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before, note that an invalid connection to druid://google.com:8082/druid/v2/sql/ times out only after ~90 seconds:

Screen Shot 2022-01-11 at 8 06 56 AM

After, with TEST_DATABASE_CONNECTION_TIMEOUT set to 2 seconds just for testing:

Screen Shot 2022-01-11 at 7 57 10 AM

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@codecov
Copy link

codecov bot commented Jan 11, 2022

Codecov Report

Merging #18001 (ae30904) into master (de3d397) will decrease coverage by 0.94%.
The diff coverage is 66.66%.

❗ Current head ae30904 differs from pull request most recent head b603df8. Consider uploading reports for the commit b603df8 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18001      +/-   ##
==========================================
- Coverage   67.07%   66.12%   -0.95%     
==========================================
  Files        1609     1569      -40     
  Lines       64903    61641    -3262     
  Branches     6866     6233     -633     
==========================================
- Hits        43535    40762    -2773     
+ Misses      19502    19280     -222     
+ Partials     1866     1599     -267     
Flag Coverage Δ
hive ?
javascript 50.86% <66.66%> (-2.91%) ⬇️
mysql 82.07% <ø> (-0.12%) ⬇️
postgres 82.12% <ø> (-0.12%) ⬇️
presto ?
python 82.21% <ø> (-0.48%) ⬇️
sqlite 81.81% <ø> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...-chart-controls/src/sections/advancedAnalytics.tsx 33.33% <ø> (ø)
...s/legacy-plugin-chart-country-map/src/countries.ts 100.00% <ø> (ø)
...legacy-plugin-chart-partition/src/controlPanel.tsx 25.00% <ø> (ø)
...gins/legacy-plugin-chart-rose/src/controlPanel.tsx 50.00% <ø> (ø)
.../plugins/legacy-preset-chart-nvd3/src/Bar/index.js 66.66% <ø> (ø)
...gins/legacy-preset-chart-nvd3/src/DistBar/index.js 66.66% <ø> (ø)
...ins/legacy-preset-chart-nvd3/src/DualLine/index.js 66.66% <ø> (ø)
...gins/legacy-preset-chart-nvd3/src/NVD3Controls.tsx 95.83% <ø> (ø)
...plugins/legacy-preset-chart-nvd3/src/ReactNVD3.jsx 0.00% <ø> (ø)
...c/SqlLab/components/TemplateParamsEditor/index.tsx 75.00% <ø> (-9.00%) ⬇️
... and 538 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de3d397...b603df8. Read the comment docs.

@john-bodley
Copy link
Member

john-bodley commented Jan 11, 2022

@betodealmeida out of interest in a slow ping problematic? I gather adding and thus testings a database connection—except if it is used for offline monitoring—is somewhat of a rare event.

Additionally are there specific database engines which may take minutes to ping? Asking to learn as the default SQLAlchemy ping is merely a SELECT 1 per here.

@betodealmeida
Copy link
Member Author

@betodealmeida out of interest in a slow ping problematic? I gather adding and thus testings a database connection—except if it is used for offline monitoring—is somewhat of a rare event.

We saw a lot of errors when people are trying to setup their databases, with a lot of people simply giving up. Because of that we're trying to make the process less error-prone, or at least quicker.

Additionally are there specific database engines which may take minutes to ping? Asking to learn as the default SQLAlchemy ping is merely a SELECT 1 per here.

When the DB connects successfully the ping is fast. The problem is that if the URL is incorrect it might take a long time for the test to fail, which only happens when the Superset <--> DB connection times out. If it takes more than 30 seconds to run SELECT 1 it's better to fail early, IMHO, because the SQLAlchemy URI is probably incorrect.

@betodealmeida betodealmeida merged commit 51090c3 into apache:master Jan 12, 2022
shcoderAlex pushed a commit to casual-precision/superset that referenced this pull request Feb 7, 2022
* feat: shorter timeout on test_connection

* pip-compile-multi --no-upgrade

* Fix for SQLite

* Return 408

* Add test
bwang221 pushed a commit to casual-precision/superset that referenced this pull request Feb 10, 2022
* feat: shorter timeout on test_connection

* pip-compile-multi --no-upgrade

* Fix for SQLite

* Return 408

* Add test
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants