chore(key-value): convert command to dao #29344

villebro · 2024-06-24T13:42:38Z

SUMMARY

This PR converts the Key Value commands to a DAO, which gives the caller much more control over when commits/rollbacks happen, and what to do about expired entries. Other changes:

Update all code that uses the KV commands to use the new DAO, making sure flushes/commits happen outside the DAO. In particular, this covers the following major components:
- Explore permalinks
- Dashboard permalinks
- Distributed lock. During the refactor, the distributed lock was moved under a module of its own + break out the stateful logic into commands that use the new DAO.
- Metastore cache. I considered breaking out the logic into commands, but it felt more sensible to keep the code as-is under a single python file, as it's under extensions. In addition, this doesn't use the new transaction decorator, as we need to adhere to the Flask-Caching API.
The old KV command integration tests are converted into functionally equivalent unit tests

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

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 · 2024-06-24T14:16:35Z

Codecov Report

Attention: Patch coverage is 93.86792% with 13 lines in your changes missing coverage. Please review.

Project coverage is 83.87%. Comparing base (76d897e) to head (d7752de).
Report is 393 commits behind head on master.

Files	Patch %	Lines
superset/distributed_lock/utils.py	64.28%	5 Missing ⚠️
superset/distributed_lock/__init__.py	85.00%	3 Missing ⚠️
superset/daos/key_value.py	97.05%	2 Missing ⚠️
superset/commands/distributed_lock/base.py	94.11%	1 Missing ⚠️
superset/commands/distributed_lock/create.py	95.23%	1 Missing ⚠️
superset/commands/distributed_lock/get.py	94.11%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #29344       +/-   ##
===========================================
+ Coverage   60.48%   83.87%   +23.38%     
===========================================
  Files        1931      519     -1412     
  Lines       76236    37409    -38827     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    31376    -14738     
+ Misses      28017     6033    -21984     
+ Partials     2105        0     -2105

Flag	Coverage Δ
hive	`49.09% <25.94%> (-0.08%)`	⬇️
javascript	`?`
mysql	`77.16% <50.94%> (?)`
postgres	`77.26% <50.94%> (?)`
presto	`53.71% <25.94%> (-0.09%)`	⬇️
python	`83.87% <93.86%> (+20.38%)`	⬆️
sqlite	`76.74% <50.94%> (?)`
unit	`59.77% <83.01%> (+2.14%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

villebro · 2024-06-24T14:40:58Z

superset/key_value/types.py

-@dataclass
-class Key:
-    id: int | None
-    uuid: UUID | None
-
-


This is no longer needed, as the DAO always returns the full entry

superset/daos/key_value.py

villebro · 2024-06-30T13:58:23Z

tests/unit_tests/fixtures/common.py

+@pytest.fixture
+def admin_user() -> User:
+    role = db.session.query(Role).filter_by(name="Admin").one()
+    user = User(
+        first_name="Alice",
+        last_name="Doe",
+        email="adoe@example.org",
+        username="alice_admin",
+        roles=[role],
+    )
+    db.session.add(user)
+    db.session.flush()
+    yield user


I tried to use the admin user that gets created during test startup, and that worked when running the key_value_test.py in isolation, but when running the full test suite the user disappears. So it appears there's a test that deletes it. I didn't have time to try to find the culprit, but at any rate I feel it may be a better idea to not rely on any pre-existing fixtures, hence I suggest using this fixture going forward for new tests needing an admin user.

villebro · 2024-06-30T17:40:55Z

superset/daos/key_value.py

+
+class KeyValueDAO(BaseDAO[KeyValueEntry]):
+    @staticmethod
+    def get_entry(


To reviewers who think the _entry suffix seems redundant: BaseDAO already has create, update, delete, which expects passing an instance of the model. This doesn't fit this use case very well, as the interface here is slightly different from the typical ORM object:

resource specifies the namespace

key can be an int or UUID

The Key Value model encodes/decodes values using a codec that we want to enforce in our method signatures

For this reason I needed to name these slightly differently to not collide with the base methods.

villebro · 2024-06-30T17:41:15Z

superset/daos/key_value.py

+        return db.session.query(KeyValueEntry).filter_by(**filter_).first()
+
+    @classmethod
+    def get_value(


This is a convenience method for when one only wants the value, not the full entry

villebro · 2024-07-01T06:30:40Z

superset/distributed_lock/__init__.py

+    value = GetDistributedLock(namespace=namespace, params=kwargs).run()
+    if value:
+        logger.debug("Lock on namespace %s for key %s already taken", namespace, key)
+        raise CreateKeyValueDistributedLockFailedException("Lock already taken")
+


I added a get before trying to create the lock, as I noticed that creating locks when it was already taken was producing ugly error logs on Postgres. While this adds one more roundtrip to the metastore, the locking operations are fairly rare, hance it felt cleaner to do it this way. Note, that we're still handling the rare case that the lock got created after the get, so that will still be handled gracefully.

villebro · 2024-07-01T06:37:08Z

tests/integration_tests/explore/permalink/commands_tests.py

            "chartId": slice.id,
            "datasetId": dataset.id,
            "datasource": datasource_string,
+            "datasourceType": DatasourceType.TABLE.value,


bycatch: the return value was missing a required field

superset/commands/explore/permalink/create.py

superset/extensions/metastore_cache.py

tests/unit_tests/dao/key_value_test.py

michael-s-molina

LGTM. Thanks for the improvements and for addressing all comments @villebro!

rtexelm · 2024-07-01T22:55:47Z

@villebro I can't pinpoint why but something in this PR has caused local dev builds to fail with a flask app_context error. Also observed by QA at Preset.

2024-07-01 17:47:46,317:ERROR:superset.app:Failed to create app
Traceback (most recent call last):
  File "/Users/user/superset/app.py", line 40, in create_app
    app_initializer.init_app()
  File "/Users/user/superset/initialization/__init__.py", line 479, in init_app
    self.configure_cache()
  File "/Users/user/superset/initialization/__init__.py", line 511, in configure_cache
    cache_manager.init_app(self.superset_app)
  File "/Users/user/superset/utils/cache_manager.py", line 93, in init_app
    self._init_cache(
  File "/Users/user/superset/utils/cache_manager.py", line 87, in _init_cache
    cache.init_app(app, cache_config)
  File "/Users/user/site-packages/flask_caching/__init__.py", line 145, in init_app
    self._set_cache(app, config)
  File "/Users/user/site-packages/flask_caching/__init__.py", line 155, in _set_cache
    cache_factory = import_string(import_me)
  File "/Users/user/site-packages/werkzeug/utils.py", line 596, in import_string
    __import__(import_name)
  File "/Users/user/superset/extensions/metastore_cache.py", line 27, in <module>
    from superset.daos.key_value import KeyValueDAO
  File "/Users/user/superset/daos/key_value.py", line 32, in <module>
    from superset.key_value.models import KeyValueEntry
  File "/Users/user/superset/key_value/models.py", line 24, in <module>
    from superset.models.helpers import AuditMixinNullable, ImportExportMixin
  File "/Users/user/superset/models/__init__.py", line 17, in <module>
    from . import core, dynamic_plugins, sql_lab, user_attributes  # noqa: F401
  File "/Users/user/superset/models/core.py", line 74, in <module>
    from superset.models.helpers import AuditMixinNullable, ImportExportMixin
  File "/Users/user/superset/models/helpers.py", line 107, in <module>
    config = app.config
  File "/Users/user/site-packages/werkzeug/local.py", line 318, in __get__
    obj = instance._get_current_object()
  File "/Users/user/site-packages/werkzeug/local.py", line 519, in _get_current_object
    raise RuntimeError(unbound_message) from None
RuntimeError: Working outside of application context.

This typically means that you attempted to use functionality that needed
the current application. To solve this, set up an application context
with app.app_context(). See the documentation for more information.

Followed by this

Traceback (most recent call last):
  File "/Users/user/bin/superset", line 8, in <module>
    sys.exit(superset())
  File "/Users/user/lib/python3.10/site-packages/click/core.py", line 1157, in __call__
    return self.main(*args, **kwargs)
  File "/Users/user/lib/python3.10/site-packages/click/core.py", line 1078, in main
    rv = self.invoke(ctx)
  File "/Users/user/lib/python3.10/site-packages/click/core.py", line 1685, in invoke
    super().invoke(ctx)
  File "/Users/user/lib/python3.10/site-packages/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/user/lib/python3.10/site-packages/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
  File "/Users/user/lib/python3.10/site-packages/click/decorators.py", line 33, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/Users/user/lib/python3.10/site-packages/flask/cli.py", line 355, in decorator
    app = __ctx.ensure_object(ScriptInfo).load_app()
  File "/Users/user/lib/python3.10/site-packages/flask/cli.py", line 309, in load_app
    app = locate_app(import_name, name)
  File "/Users/user/lib/python3.10/site-packages/flask/cli.py", line 238, in locate_app
    return find_app_by_string(module, app_name)
  File "/Users/user/lib/python3.10/site-packages/flask/cli.py", line 166, in find_app_by_string
    app = attr(*args, **kwargs)
  File "/Users/user/superset/app.py", line 40, in create_app
    app_initializer.init_app()
  File "/Users/user/superset/initialization/__init__.py", line 479, in init_app
    self.configure_cache()
  File "/Users/user/superset/initialization/__init__.py", line 511, in configure_cache
    cache_manager.init_app(self.superset_app)
  File "/Users/user/superset/utils/cache_manager.py", line 93, in init_app
    self._init_cache(
  File "/Users/user/superset/utils/cache_manager.py", line 87, in _init_cache
    cache.init_app(app, cache_config)
  File "/Users/user/lib/python3.10/site-packages/flask_caching/__init__.py", line 145, in init_app
    self._set_cache(app, config)
  File "/Users/user/lib/python3.10/site-packages/flask_caching/__init__.py", line 155, in _set_cache
    cache_factory = import_string(import_me)
  File "/Users/user/lib/python3.10/site-packages/werkzeug/utils.py", line 596, in import_string
    __import__(import_name)
  File "/Users/user/superset/extensions/metastore_cache.py", line 27, in <module>
    from superset.daos.key_value import KeyValueDAO
  File "/Users/user/superset/daos/key_value.py", line 32, in <module>
    from superset.key_value.models import KeyValueEntry
  File "/Users/user/superset/key_value/models.py", line 24, in <module>
    from superset.models.helpers import AuditMixinNullable, ImportExportMixin
  File "/Users/user/superset/models/__init__.py", line 17, in <module>
    from . import core, dynamic_plugins, sql_lab, user_attributes  # noqa: F401
  File "/Users/user/superset/models/core.py", line 74, in <module>
    from superset.models.helpers import AuditMixinNullable, ImportExportMixin
  File "/Users/user/superset/models/helpers.py", line 107, in <module>
    config = app.config
  File "/Users/user/lib/python3.10/site-packages/werkzeug/local.py", line 318, in __get__
    obj = instance._get_current_object()
  File "/Users/user/lib/python3.10/site-packages/werkzeug/local.py", line 519, in _get_current_object
    raise RuntimeError(unbound_message) from None
RuntimeError: Working outside of application context.

This typically means that you attempted to use functionality that needed
the current application. To solve this, set up an application context
with app.app_context(). See the documentation for more information.

villebro · 2024-07-02T05:53:41Z

@rtexelm I was able to reproduce, it was due to a last second cleanup where I moved imports to the top level from the methods. I'll open up a fix shortly after doing some more testing.

chore(key-value): convert command to dao

59c7329

pull-request-size bot added the size/XXL label Jun 24, 2024

villebro force-pushed the villebro/key-value-dao branch from 8b304d0 to d78c211 Compare June 24, 2024 13:59

add missing flush + delint

997efda

villebro force-pushed the villebro/key-value-dao branch from d78c211 to 997efda Compare June 24, 2024 14:09

add pylint ignore

7ab1f48

villebro mentioned this pull request Jun 24, 2024

chore(dao/command): Add transaction decorator to try to enforce "unit of work" #24969

Merged

9 tasks

remove redundant dataclass

d3d3565

villebro commented Jun 24, 2024

View reviewed changes

michael-s-molina requested a review from john-bodley June 24, 2024 14:43

john-bodley reviewed Jun 24, 2024

View reviewed changes

superset/daos/key_value.py Show resolved Hide resolved

john-bodley reviewed Jun 24, 2024

View reviewed changes

superset/daos/key_value.py Outdated Show resolved Hide resolved

villebro added 2 commits June 30, 2024 16:21

Merge branch 'master' into villebro/key-value-dao

5c0c36b

reintroduce admin fixture

fec71db

villebro commented Jun 30, 2024

View reviewed changes

villebro added 2 commits June 30, 2024 17:12

reintroduce separate session for assertions

c727602

remove redundant commit

515dc80

villebro commented Jun 30, 2024

View reviewed changes

villebro force-pushed the villebro/key-value-dao branch from 4e7c75f to 0aa7116 Compare July 1, 2024 06:05

use type alias for key

2859dee

villebro force-pushed the villebro/key-value-dao branch from 0aa7116 to 2859dee Compare July 1, 2024 06:20

villebro commented Jul 1, 2024

View reviewed changes

villebro added 3 commits July 1, 2024 09:31

fix exception description

f52397a

simplify by using KeyValueDAO.get_value

193b026

use transaction

028e139

villebro commented Jul 1, 2024

View reviewed changes

move imports and fix get for expired values

7579c03

villebro requested a review from michael-s-molina July 1, 2024 07:47

michael-s-molina reviewed Jul 1, 2024

View reviewed changes

superset/commands/explore/permalink/create.py Outdated Show resolved Hide resolved

michael-s-molina reviewed Jul 1, 2024

View reviewed changes

superset/extensions/metastore_cache.py Outdated Show resolved Hide resolved

michael-s-molina reviewed Jul 1, 2024

View reviewed changes

tests/unit_tests/dao/key_value_test.py Outdated Show resolved Hide resolved

villebro added 2 commits July 1, 2024 17:23

address comments

1a88e13

add after_each fixture

42281de

villebro requested a review from michael-s-molina July 1, 2024 15:49

simplify

d7752de

michael-s-molina approved these changes Jul 1, 2024

View reviewed changes

villebro merged commit 7d6e933 into apache:master Jul 1, 2024
37 checks passed

villebro deleted the villebro/key-value-dao branch July 1, 2024 17:23

villebro mentioned this pull request Jul 2, 2024

fix(metastore-cache): import dao in methods #29451

Merged

9 tasks

eschutho pushed a commit that referenced this pull request Jul 24, 2024

chore(key-value): convert command to dao (#29344)

a77d4e7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(key-value): convert command to dao #29344

chore(key-value): convert command to dao #29344

villebro commented Jun 24, 2024 •

edited

Loading

codecov bot commented Jun 24, 2024 •

edited

Loading

villebro Jun 24, 2024

villebro Jun 30, 2024

villebro Jun 30, 2024 •

edited

Loading

villebro Jun 30, 2024

villebro Jul 1, 2024

villebro Jul 1, 2024

michael-s-molina left a comment

rtexelm commented Jul 1, 2024 •

edited

Loading

villebro commented Jul 2, 2024

chore(key-value): convert command to dao #29344

chore(key-value): convert command to dao #29344

Conversation

villebro commented Jun 24, 2024 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jun 24, 2024 • edited Loading

Codecov Report

villebro Jun 24, 2024

Choose a reason for hiding this comment

villebro Jun 30, 2024

Choose a reason for hiding this comment

villebro Jun 30, 2024 • edited Loading

Choose a reason for hiding this comment

villebro Jun 30, 2024

Choose a reason for hiding this comment

villebro Jul 1, 2024

Choose a reason for hiding this comment

villebro Jul 1, 2024

Choose a reason for hiding this comment

michael-s-molina left a comment

Choose a reason for hiding this comment

rtexelm commented Jul 1, 2024 • edited Loading

villebro commented Jul 2, 2024

villebro commented Jun 24, 2024 •

edited

Loading

codecov bot commented Jun 24, 2024 •

edited

Loading

villebro Jun 30, 2024 •

edited

Loading

rtexelm commented Jul 1, 2024 •

edited

Loading