Test data warehouse connections on checkout #11018
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
[ci drivers]
[ci sparksql]
[ci sparksql]
This was referenced Oct 3, 2019
This was referenced Oct 7, 2019
camsaul
added a commit
that referenced
this pull request
Mar 13, 2020
* update version * Standardize form presentation (#10673) * clean up Form-input hover state * standardize on one form style, remove old styles from use * update snapshot test * fix forgot password position * fix form error css * fix padding on setup flow to account for offset removal * fix layout on admin database add/edit * increase admin db form padding * cleanup scheduler * fix placeholder text * tweak label and field weight and border contrast * error message separator * match select style in Form-field to other fields * fix admin / database edit form width * use updated input style on admin/settings * clean up whitespace in database details form * (re)Merge release 0.33.x into master (#10835) * Don't fingerprint sensitive fields (#10776) * Druid: fix day bucketing (#10782) * Load initial Docker Metabase db from provided H2 dump (#10834) * Load initial Docker Metabase db from provided H2 dump * Move H2 init db to /app and add `mv.db` extension * Remove `COPY` of init db in Dockerfile in favor of volume * Document how to mount initial db in container * Don't load H2 db into a populated db * Add example Dockerfile that copies in initialization db * Switch to clojure.test (#10863) * use the dimension column for getXValues and getParseOptions (#10827) * [cmd] dump to h2 (#10843) * Fix (most of the) driver tests after merging PR to switch to clojure.test (#10882) * Clarify microcopy for Do Not Include column metadata setting (#10889) * clarify copy for Do Not Include setting * update test * fix another test * Enable eqeqeq eslint rule (#10888) * Run apt-get update before apt-get install gettext (#10903) * Fix CI test failures (#10904) * Test tweaks (#10925) * v0.33.3 * Cover visualization with "mouse blocker" if dashboard is being edited (#10931) * catch fullscreen error and mention limitation in docs (#10896) * alternative implemenation of permission graph conversion (#10919) * simple fixes for data permissions, new model for conforming permissions * collection permission conversion * only assoc values if they're present * improve docs * progress on testing permissions changing * test that setting permissions on new table works * remove permission graph reworking to introduce in another branch * clean ns * alternative impl of permission graph conversion * correct the permission graph specs * fix whitespace * address ci failure * undo ci config change * delete mysteriously undeleted merge conflict * Fix bugs with stacked charts and log scales (#10911) * compute yExtents for stacked charts, display a better min for log bar charts * remove unused const * Fix bug saving database info in reference (#10940) * database -> Databases * add e2e test * unused reference * clean up after test * create/delete db rather than trying to reset fields * Documenting the ability to disable password log-in [ci skip] (#10887) * [style] whitespace (#10937) * Don't run native queries with invalid template tags (#10946) * don't run native queries with invalid template tags * fix test * change setCardAndRun to onChangeCardAndRun in LeafletMap (#10917) * Don't don't set dimensions in ExplicitSize until after initial mounting (#10890) * don't don't set dimensions in ExplicitSize until after initial mounting * add comment * add delay to fix tests * Fix how optional field filter params are substituted; fix support for nested optional params (#10947) * Param fixes [ci skip] * Almost all working [ci skip] * Test fixes * Test fixes * Test / linter fixes * Fix linter errors * Fix filtering by month-of-year and month in MongoDB; lots of new tests (#10955) * Fix filtering by month-of-year and month in MongoDB; lots of new tests [ci drivers] * Test fixes 🔧 [ci drivers] * Test fixes! [ci drivers] * Druid: set correct type to complex aggregators in post aggregation (#10836) Druid: set correct type to complex aggregators in post aggregation * Improve qb startup performance (#10948) * reliably remove mouse blocker div (#10964) * Add support for metabase.util.date/date-trunc for :seconds unit. (#10978) * Move MBQL filter negation/desugaring logic to MBQL lib (#10977) * 4779 locked params (#10878) * starting to put together warning component * progress toward embed code warnings * prettify * sync params/embedding params, show warning * prety * comment goodness * fix types I think? * trying to get the flow types right * finish fixing types * pretty * implement tom's cleanup suggestions * prettier again * fix issue with editing params multiple times * prettier * fix issue with 'adding dashcard' ruining my carefully crafted embed warning code * handle empty dashboards * prettier * handle case where there are no embedded params * Revert "fix issue with 'adding dashcard' ruining my carefully crafted embed warning code" This reverts commit 50852b8. * restore behavior of entering edit mode after adding card * implement tom's suggestion for editwarning * unit tests for syncParametersAndEmbeddingParams * Some small changes to the permissions & collections docs [ci skip] (#10998) * Only show line/area/bar buttons for line, area, bar, or combo charts (#10979) * some ie11 fixes (#10993) * Fix tooltip bug in pie charts (#10994) * Optimize datetime filters! (#10980) The biggest performance improvement in history of Metabase since we switched to Clojure Before when filtering by a "bucketed" datetime field we would wrap the field and the value(s) it was being compared against in casting/truncation functions. For example when compiling a query to find fields where month is September 2019, we previously generated SQL like this: ```sql SELECT count(*) FROM public.checkins WHERE date_trunc('month', CAST(public.checkins.date AS timestamp)) = CAST(timestamp '2019-09-01T00:00:00Z' AS date) ``` As mentioned in #4043, this is not good for performance! Metabase now generates logically equivalent SQL like ```sql SELECT count(*) FROM public.checkins WHERE ( public.checkins.date >= timestamp '2019-09-01T00:00:00Z' AND public.checkins.date < timestamp '2019-10-01T00:00:00Z' ) ``` Fixes #4043 Fixes #11009 Fixes #11010 Fixes #11011 Fixes #11012 Fixes #11013 😮 🏎 * add hover style to question description (#11017) * Test data warehouse connections on checkout (#11018) c3p0 has an option to test connections when fetched from the connection pool intended to fix that exact problem, so this was a simple matter of enabling it. It is not enabled by default because it adds overhead to connection checkout, especially for JDBC drivers that don't support the full JDBC 4 API, which includes a Connection.isValid() method. However, all of our JDBC drivers are JDBC 4 compliant, meaning c3p0 can check connection validity fairly efficiently. With the data warehouse on the same machine as the Metabase server, the added overhead was in the order of ~100µs, or an order of magnitude or two below the threshold of human perception. With the data warehouse in AWS us-east-1 and the Metabase server in San Francisco, the overhead was around ~70ms, which is basically typical network latency for such a request. Based on those results, it appears that the additional cost of this test is roughly equal to the network latency of the request. IRL the Metabase server and data warehouse are likely to be located in closer geographical proximity to one another than my trans-contintental tests, if not in the same AWS region (or equivalent). I would expect the added overhead for most query executions for most setups to be in the 1-10ms range and the p99 to be under 100ms extra latency. Accepting that overhead seems like a no-brainer when considering that an entire class of bugs will be fixed by enabling the option. * BigQuery code improvements (#11024) * Fix legacy auto datetime bucketing filtering against relative-datetime (#11025) * fix confirm modal footer padding (#11020) * remove extra padding on confirm modal * add modal examples to help keep an eye on things * Bundle fonts (#10963) * remove Google Fonts usage * local Lato in resources/frontend_client/app/fonts * create assets/fonts aliases, update publicPath, use absolute paths in css * add svg to extensions * update flow config to account for frontend_client assets * add explicit .svg extension to no_results usage * remove google fonts references from CSP headers * Align platform parameter in JS redirect (#11031) Fixes #10523. * Full support for all dimension types in expression editor (#10981) * Full support for all dimension types in expression editor (field-id, field-literal, fk->, joined-field, expression) * Fix various bugs in metabase-lib * Limit 'card-has-ambiguous-columns?' to native queries + use real DB id in composeThisQuery * replace metadata.database[...] etc with metadata.database(...) * Fix and add to tests * Revert "replace metadata.database[...] etc with metadata.database(...)" This reverts commit 0f5aa69. * Fix joined dimension options, e.x. custom field from a saved query * Update screenshot in readme (#10952) * call onResize during onResizeStart to set state (#10951) * Fix public questions with required parameters (#10794) * Fix parameter splicing when converting queries to SQL (#11038) * Don't fail entire query if we don't know how to convert value to literal (#11046) * Don't fail entire query if we don't know how to convert result to literal * Code cleanup 🚿 * Don't use column settings when formatting pie chart legend percentages (#10962) * show am/pm if locale uses 12-hour clock (#11049) * update password complexity statement for more accuracy (#11041) The current text implies that a password must be _exactly_ `complexity.total` length, whereas that is actually the minimum length. * Remove .git from .dockerignore so bin/version gets the correct branch/hash (#11061) * Clean up and reorder AWS EB install steps (#11050) * clean up and reorder install steps [ci skip] * correct the upload-your-code instructions [ci skip] * Correct suggested value for "availability" * Fix bullets and quoting * Add periods at end of sentences * Fix grammar and capitalization * Specify config blocks that options are from * Align emphasis of "NOTE" lines * Fix more bullets * Fix one more NOTE * some small tweaks and screenshot updates [ci skip] * Test for #7435 [ci bigquery] (#11060) * Test to help debug #9844 (#11068) * Improve entities performance (#11040) * Delegate getList to getObject * Use getMetadata for metadata entities getObject selectors * Per object caching in getObject using re-reselect * Validate trs/tru args counts at macroexpansion time (#11045) * Validate i18n at compile time; fix busted format strings 🔧 * Lint fixes 🔧 * Don't cache DB -> driver forever; fixes #9844 (probably) (#11078) * require custom expression name (#11065) * Fix redshift tests [ci redshift] (#11083) * add "unsubscribe from pulse" endpoint (#10688) add an endpoint so that users who don't own a pulse can unsubscribe themselves from the pulse * Drill through fixes and cleanup (#11032) * Full support for all dimension types in expression editor (field-id, field-literal, fk->, joined-field, expression) * Fix various bugs in metabase-lib * Limit 'card-has-ambiguous-columns?' to native queries + use real DB id in composeThisQuery * Fix and add to tests * Swap Question constructor metadata/card args + misc other cleanup * Refactor and fix drill-throughs to support explicit joins etc * Misc FE metadata code cleanup (#11044) * Fix joined dimension options, e.x. custom field from a saved query * Cleanup schema_metadata and related method names * Use shorter aggregate/filter/breakout/sort/join methods everywhere * Add UnderlyingRecordsDrill tests * Bump metabase/connection-pool dep version (#11085) * v0.33.4 * Update version * Fix typo [ci drivers] * Fix order of new Question() args in audit app * Test fixes [ci drivers] * test fix [ci drivers] * Test fixes * Update settings.js * Update GTAPModal.jsx * Test fixes 🔧 [ci drivers]
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.
Fixes #9989
c3p0 has an option to test connections when fetched from the connection pool intended to fix that exact problem, so this was a simple matter of enabling it. It is not enabled by default because it adds overhead to connection checkout, especially for JDBC drivers that don't support the full JDBC 4 API, which includes a
Connection.isValid()method. However, all of our JDBC drivers are JDBC 4 compliant, meaning c3p0 can check connection validity fairly efficiently.As someone who used to write big chunks of my iOS apps in C++ for performance, I am not one to enable a flag that adds this sort of overhead without a second thought. But rest assured; this change was carefully considered and profiled extensively to assuage my own concerns about its impact.
Profiling Results
With the data warehouse on the same machine as the Metabase server, the added overhead was in the order of ~100µs, or an order of magnitude or two below the threshold of human perception. With the data warehouse in AWS
us-east-1and the Metabase server in San Francisco, the overhead was around ~70ms, which is basically typical network latency for such a request.Based on those results, it appears that the additional cost of this test is roughly equal to the network latency of the request. IRL the Metabase server and data warehouse are likely to be located in closer geographical proximity to one another than my trans-contintental tests, if not in the same AWS region (or equivalent). I would expect the added overhead for most query executions for most setups to be in the 1-10ms range and the p99 to be under 100ms extra latency.
Accepting that overhead seems like a no-brainer when considering that an entire class of bugs will be fixed by enabling the option.