fix: Issue 13956 #13980

john-bodley · 2021-04-07T02:10:25Z

SUMMARY

Fixes #13956. More specifically the issue with "caching" the the dataset health check message (#11970) on the associated datasource record was that if the callback was defined, the first time a user when to explorer the record was updated with the callback response where the user was defined as the updater.

Additionally tying the health check solely to the metadata of the datasource is likely too restrictive, i.e., it seems plausible that an institution may want to have custom logic which actually introspects the underlying data. Hence the fix was to remove the caching on the datasource record and instead place on ownership on the institution to write the necessary logic for memoizing the callback (which includes the specific datasource as an argument).

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

CI.

ADDITIONAL INFORMATION

Has associated issue:
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 · 2021-04-07T02:39:12Z

Codecov Report

Merging #13980 (49b86a6) into master (667eb83) will decrease coverage by 0.10%.
The diff coverage is 85.71%.

❗ Current head 49b86a6 differs from pull request most recent head 57f4d74. Consider uploading reports for the commit 57f4d74 to get more accurate results

@@            Coverage Diff             @@
##           master   #13980      +/-   ##
==========================================
- Coverage   79.40%   79.30%   -0.11%     
==========================================
  Files         938      938              
  Lines       47541    47523      -18     
  Branches     5940     5940              
==========================================
- Hits        37749    37687      -62     
- Misses       9666     9710      +44     
  Partials      126      126

Flag	Coverage Δ
cypress	`56.05% <ø> (ø)`
hive	`80.45% <85.71%> (-0.04%)`	⬇️
mysql	`80.72% <85.71%> (-0.04%)`	⬇️
postgres	`80.75% <85.71%> (-0.04%)`	⬇️
presto	`?`
python	`81.16% <85.71%> (-0.19%)`	⬇️
sqlite	`80.36% <85.71%> (-0.04%)`	⬇️

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

Impacted Files	Coverage Δ
superset/views/core.py	`75.63% <ø> (-0.04%)`	⬇️
superset/views/datasource.py	`88.33% <ø> (-0.38%)`	⬇️
superset/config.py	`90.65% <50.00%> (-0.32%)`	⬇️
superset/connectors/sqla/models.py	`88.68% <100.00%> (-2.10%)`	⬇️
superset/datasets/dao.py	`96.62% <100.00%> (-0.05%)`	⬇️
superset/db_engine_specs/presto.py	`84.10% <0.00%> (-5.44%)`	⬇️
superset/models/core.py	`89.10% <0.00%> (-0.28%)`	⬇️

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 667eb83...57f4d74. Read the comment docs.

craig-rueda · 2021-04-07T02:58:25Z

superset/migrations/versions/134cea61c5e7_remove_dataset_health_check_message.py

+
+
+def downgrade():
+    pass


Can we impl a down migration here?

There's nothing to downgrade to, i.e., the upgrade removes all the key and associated values from the extra blob.

Right, but if one needed to revert, would that even make sense to put the keys back?

@craig-rueda the key is optional and thus unnecessary to re-add if a downgrade was performed.

etr2460 · 2021-04-07T03:43:15Z

superset/config.py

-DATASET_HEALTH_CHECK = None
+# A SQL dataset health check. Note if enabled the callable should be cached to aid with
+# performance.
+DATASET_HEALTH_CHECK: Optional[Callable[["SqlaTable"], str]] = None


this should probably be a BaseDatasource not a SqlaTable?

@etr2460 this functionality is only provided for the SQLA connector.

Would be nice do add an health check callable example here, or add it to the docs

@dpgaspar I added an example.

etr2460 · 2021-04-07T03:44:51Z

superset/connectors/sqla/models.py

+        check = config["DATASET_HEALTH_CHECK"]
+        return check(self) if check else None


what's more pythonic? this or:

Suggested change

check = config["DATASET_HEALTH_CHECK"]

return check(self) if check else None

return (config["DATASET_HEALTH_CHECK"] or lambda: None)(self)

is this even valid python? 🤷

graceguo-supercat · 2021-04-07T06:02:13Z

superset/config.py

-# It will get executed each time when user open a chart's explore view.
-DATASET_HEALTH_CHECK = None
+# A SQL dataset health check. Note if enabled the callable should be cached to aid with
+# performance.


If the healthy status is in cache but not persist in database, when restart server will we lost all the status?

right now we can run some queries to see healthy status. if use cache solution we will not be easy to know this number?

@graceguo-supercat:

The Redis (or similar) cache rarely is restarted.

It's definitely less clear though possible via a Redis (or similar) query.

ktmud

The solution makes sense. I guess the lesson is there should be less implicit side-effects.

ktmud · 2021-04-07T06:14:22Z

superset/connectors/sqla/models.py

-                if config.get("DATASET_HEALTH_CHECK")
-                else None
-            )
+            data_["health_check_message"] = self.health_check_message


Suggested change

data_["health_check_message"] = self.health_check_message

data_["health_check_message"] = self.check_health()

If this will run an external function every time this line is visited, maybe it's better to change this property to an actual function call so it "looks" more expensive...

@ktmud I thought about that. I think we could also just use @functools.lru_cache for the property so at least the property is cached locally.

graceguo-supercat · 2021-04-07T16:26:44Z

@dpgaspar Do you have any suggestion for this issue and fix? When a user open explore view to see a chart, we want to add a health check for the sql in the datasource, and want to save the check results into dataset extra column so that don't run the check every time. The problem is, this update for "extra" will set the viewer of chart as the last updater for dataset. Is there anyway to avoid this behavior? thanks!

robdiciuccio · 2021-04-07T16:53:17Z

Removing the health_check property in the extra JSON schema seems to qualify as a breaking change according to SIP-57. Can the migration be broken out as a separate PR and staged for cleanup in the next major release (SIP-59)? Is the migration strictly necessary to re-implement the datasource health checks?

john-bodley · 2021-04-07T17:17:59Z

@robdiciuccio I can break the migration into a separate PR.

dpgaspar

Just a comment to improve docs

dpgaspar · 2021-04-07T17:44:59Z

superset/config.py

-DATASET_HEALTH_CHECK = None
+# A SQL dataset health check. Note if enabled the callable should be cached to aid with
+# performance.
+DATASET_HEALTH_CHECK: Optional[Callable[["SqlaTable"], str]] = None


Would be nice do add an health check callable example here, or add it to the docs

ktmud · 2021-04-07T17:53:50Z

On second thought, I'm still a little torn between having Superset manage the cache vs shifting the responsibility to the end users. IMO the config file should be as lightweight as possible. The more logics we force users to put in there, the more likely things would break.

I'm wondering whether we can utilize sqla's before_insert and before_update events to run health checks on db updates instead of calling check_health() in views, so that the check function won't trigger a new update? For existing datasets, we provide a CLI command for admins to run health checks on all of them in batch.

dpgaspar · 2021-04-07T17:56:42Z

@dpgaspar Do you have any suggestion for this issue and fix? When a user open explore view to see a chart, we want to add a health check for the sql in the datasource, and want to save the check results into dataset extra column so that don't run the check every time. The problem is, this update for "extra" will set the viewer of chart as the last updater for dataset. Is there anyway to avoid this behavior? thanks!

I haven't tested it but explicitly setting the changed_by_fk on a separated merge maybe?

john-bodley · 2021-04-07T18:12:29Z

@ktmud I prefer the proposed approach as it provides the user with a lot more flexibility and per the PR description it means that the health check can relate to the actual underlying data and just not the datasource metadata. Squeezing cached datasource health check information into the datasource record (the current approach) has pros and cons.

Additionally by defining a caching strategy one can leverage Flask-Caching to clear the cache whenever the function byte-code changes and thus there's no longer a requirement for the user to ensure that they'll need to bump the version.

Finally in terms of making the config as light as possible, this hasn't been the case in the past. Superset historically has provided endless flexibility (via callbacks etc.) in the configs which places the ownership on the user which can be problematic if implemented incorrectly, but the upside can be tremendous.

graceguo-supercat · 2021-04-07T18:21:31Z

I haven't tested it but explicitly setting the changed_by_fk on a separated merge maybe?

db migration can only fix the old records right? right now the behavior is: when user opens explore view, health check function will add some message in the "extra" column, and the dataset's changed_by_fk will be changed to chart viewer.

john-bodley · 2021-04-07T21:53:22Z

Actually @robdiciuccio reading through SIP-57 this change is non-breaking, i.e., the functionality remains unchanged, though performance could be degraded if the callback function is expensive and non-cached.

john-bodley · 2021-04-08T07:15:43Z

@dpgaspar @etr2460 @graceguo-supercat @ktmud et al. this is now ready for re-review. Note although this fix is not perfect it does:

Remedy the issue in a non-breaking manner. This issue is somewhat egregious as it could pollute the metadata database with incorrect updater information which is then difficult to remedy.
Provide an equally (or possibly more) efficient implementation for caching the health checks.
Provides a more flexible and powerful health check which can leverage the physical data as opposed to only the metadata. 4. The health check isn't tied to explore etc. meaning it can be exposed anywhere.

ktmud · 2021-04-08T18:41:49Z

superset/config.py

+#
+#        if cache_manager.cache.get(name) != code:
+#            cache_manager.cache.delete_memoized(func)
+#            cache_manager.cache.set(name, code, timeout=0)


Nice example!

ktmud · 2021-04-08T18:46:08Z

superset/views/core.py

@@ -734,7 +734,7 @@ def explore(  # pylint: disable=too-many-locals,too-many-return-statements,too-m

            # if feature enabled, run some health check rules for sqla datasource
            if hasattr(datasource, "health_check"):
-                datasource.health_check()
+                datasource.health_check_message = datasource.health_check()


Does this override the default @property getter or just update the cache?

@ktmud good call. Lines 735–737 can actually be removed.

ktmud · 2021-04-08T18:49:24Z

superset/views/core.py

@@ -734,7 +734,7 @@ def explore(  # pylint: disable=too-many-locals,too-many-return-statements,too-m

            # if feature enabled, run some health check rules for sqla datasource
            if hasattr(datasource, "health_check"):


Wouldn't this be always false because you remove the health_check method/attribute from SqlaDatasource?

ktmud · 2021-04-08T18:50:53Z

superset/migrations/versions/134cea61c5e7_remove_dataset_health_check_message.py

+    session = db.Session(bind=bind)
+
+    for datasource in session.query(SqlaTable):
+        if datasource.extra:


Suggested change

if datasource.extra:

if datasource.extra and "health_check" in datasource.extra:

This should limit following ops to a smaller subset.

This is risky as your checking for the existence of a substring in a JSON encoded string rather than the existence of a key.

I don't think it is. It at least saves json.loads op for datasources without health_check info, but can be guaranteed to be correct as long as you keep all following steps.

To really save some database IO, we can do the pre-filtering in the SQLA query step:

for datasource in session.query(SqlaTable).filter( SqlaTable.extra.like('%"health_check"%') ):

Granted the downstream logic ensures that the string is actually from a JSON key (I misspoke earlier). That said this migration only takes a few seconds and thus I think the existing logic is fine.

Fine by me as well. Just thought it was a good practice to always do pre-filtering when running db migrations.

Co-authored-by: John Bodley <john.bodley@airbnb.com> (cherry picked from commit a3b41e2)

Co-authored-by: John Bodley <john.bodley@airbnb.com>

john-bodley requested a review from a team as a code owner April 7, 2021 02:10

pull-request-size bot added the size/L label Apr 7, 2021

john-bodley requested review from etr2460, graceguo-supercat and ktmud April 7, 2021 02:10

john-bodley force-pushed the john-bodley--fix-13956 branch 2 times, most recently from a16244a to 7d7d16e Compare April 7, 2021 02:28

john-bodley force-pushed the john-bodley--fix-13956 branch from 7d7d16e to 2dcb5cf Compare April 7, 2021 02:48

craig-rueda reviewed Apr 7, 2021

View reviewed changes

etr2460 reviewed Apr 7, 2021

View reviewed changes

graceguo-supercat reviewed Apr 7, 2021

View reviewed changes

ktmud approved these changes Apr 7, 2021

View reviewed changes

dpgaspar reviewed Apr 7, 2021

View reviewed changes

john-bodley force-pushed the john-bodley--fix-13956 branch 2 times, most recently from b009f90 to 9ab9ca8 Compare April 7, 2021 20:20

john-bodley closed this Apr 7, 2021

john-bodley reopened this Apr 7, 2021

john-bodley force-pushed the john-bodley--fix-13956 branch 4 times, most recently from e769fec to 6164d19 Compare April 8, 2021 00:28

john-bodley force-pushed the john-bodley--fix-13956 branch 6 times, most recently from 6db8674 to f5bbaf9 Compare April 8, 2021 07:09

john-bodley force-pushed the john-bodley--fix-13956 branch 2 times, most recently from df92188 to 9a7a7fe Compare April 8, 2021 18:34

ktmud reviewed Apr 8, 2021

View reviewed changes

john-bodley force-pushed the john-bodley--fix-13956 branch 5 times, most recently from 3d4b74a to cc89ef5 Compare April 9, 2021 01:37

fix: Issue 13956

57f4d74

john-bodley force-pushed the john-bodley--fix-13956 branch from cc89ef5 to 57f4d74 Compare April 9, 2021 02:17

john-bodley merged commit a3b41e2 into apache:master Apr 9, 2021

john-bodley added a commit to airbnb/superset-fork that referenced this pull request Apr 9, 2021

fix: Issue 13956 (apache#13980)

6c9bd34

Co-authored-by: John Bodley <john.bodley@airbnb.com> (cherry picked from commit a3b41e2)

allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021

fix: Issue 13956 (apache#13980)

07d5726

Co-authored-by: John Bodley <john.bodley@airbnb.com>

QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021

fix: Issue 13956 (apache#13980)

ca9bfea

Co-authored-by: John Bodley <john.bodley@airbnb.com>

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024

		check = config["DATASET_HEALTH_CHECK"]
		return check(self) if check else None

	check = config["DATASET_HEALTH_CHECK"]
	return check(self) if check else None
	return (config["DATASET_HEALTH_CHECK"] or lambda: None)(self)

	data_["health_check_message"] = self.health_check_message
	data_["health_check_message"] = self.check_health()

		@@ -734,7 +734,7 @@ def explore( # pylint: disable=too-many-locals,too-many-return-statements,too-m

		# if feature enabled, run some health check rules for sqla datasource
		if hasattr(datasource, "health_check"):

	if datasource.extra:
	if datasource.extra and "health_check" in datasource.extra:

fix: Issue 13956 #13980

fix: Issue 13956 #13980

Conversation

john-bodley commented Apr 7, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

codecov bot commented Apr 7, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graceguo-supercat commented Apr 7, 2021 • edited Loading

robdiciuccio commented Apr 7, 2021

john-bodley commented Apr 7, 2021

dpgaspar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud commented Apr 7, 2021 • edited Loading

dpgaspar commented Apr 7, 2021

john-bodley commented Apr 7, 2021

graceguo-supercat commented Apr 7, 2021 • edited Loading

john-bodley commented Apr 7, 2021

john-bodley commented Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley commented Apr 7, 2021 •

edited

Loading

codecov bot commented Apr 7, 2021 •

edited

Loading

graceguo-supercat commented Apr 7, 2021 •

edited

Loading

ktmud commented Apr 7, 2021 •

edited

Loading

graceguo-supercat commented Apr 7, 2021 •

edited

Loading

john-bodley commented Apr 8, 2021 •

edited

Loading