feat(GAQ): Add Redis Sentinel Support for Global Async Queries #29912

nsivarajan · 2024-08-10T17:58:50Z

SUMMARY

This PR introduces a feature that allows the Async Query Manager to use a configured cache backend through the new GLOBAL_ASYNC_QUERIES_CACHE_BACKEND setting. To maintain backward compatibility, the existing GLOBAL_ASYNC_QUERIES_REDIS_CONFIG setting is still supported but will be deprecated in the future. Additionally, this update introduces support for Redis Sentinel caching, which eliminates the single point of failure associated with the previous standalone Redis configuration.

References taken from this Idea #28839

TESTING INSTRUCTIONS

Configure GLOBAL_ASYNC_QUERIES_CACHE_BACKEND in config.py or superset_config.py with the appropriate properties:
Set CACHE_TYPE to "RedisCache" for Redis cache backend.
Set CACHE_TYPE to "RedisSentinelCache" for Redis Sentinel cache backend.
Set CACHE_TYPE to None to fall back on the previous GLOBAL_ASYNC_QUERIES_REDIS_CONFIG.

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-08-10T20:30:20Z

Codecov Report

Attention: Patch coverage is 67.94872% with 25 lines in your changes missing coverage. Please review.

Project coverage is 83.64%. Comparing base (76d897e) to head (4cc558a).
Report is 646 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/async_events/cache_backend.py	59.45%	15 Missing ⚠️
superset/async_events/async_query_manager.py	69.69%	10 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #29912       +/-   ##
===========================================
+ Coverage   60.48%   83.64%   +23.15%     
===========================================
  Files        1931      529     -1402     
  Lines       76236    38268    -37968     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    32009    -14105     
+ Misses      28017     6259    -21758     
+ Partials     2105        0     -2105

Flag	Coverage Δ
hive	`48.91% <43.58%> (-0.26%)`	⬇️
javascript	`?`
mysql	`76.73% <67.94%> (?)`
postgres	`76.80% <67.94%> (?)`
presto	`53.49% <61.53%> (-0.31%)`	⬇️
python	`83.64% <67.94%> (+20.15%)`	⬆️
sqlite	`76.27% <67.94%> (?)`
unit	`60.25% <44.87%> (+2.63%)`	⬆️

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.

nsivarajan · 2024-08-12T14:32:27Z

HI @villebro , Could you please review this PR? It was created based on the idea discussed #28839

However, I recently noticed that there is a new SIP in the design phase to modernize the framework around the same topic: #29839 . I'm not sure if this PR will still add value before the new proposal is rolled out, but I would appreciate your feedback.

villebro · 2024-08-12T17:01:58Z

@nsivarajan thanks for the PR - I think this PR is welcomed even if there's ongoing work to modernize the async task framework. Looks generally good, but let me do a proper review shortly (either today or tomorrow)

nsivarajan · 2024-08-15T01:30:39Z

@villebro Thanks for the feedback! Looking forward to your review.

nsivarajan · 2024-08-22T18:35:52Z

Hi @villebro,

I wanted to follow up on this PR. I understand you're busy, but I was hoping to get your review when you have a moment. Please let me know if there's any additional information I can provide or if there are any specific areas you'd like me to address.

Thanks again for your time and feedback!

rusackas · 2024-08-22T19:30:13Z

@nsivarajan thank you for your patience in the meantime... good to see that CI is passing and there's no conflicts still, at least! :D

villebro

Thanks so much @nsivarajan for this beautiful PR (the test coverage here is great)! Sorry for the long review time - there was a lot to take in, and I may need to do a new round after I get some feedback on these comments. Please let me know what you think, and I'll do my best to do a second (and hopefully final) pass shortly.

villebro · 2024-08-12T22:31:00Z

superset/async_events/async_query_manager.py

@@ -16,18 +16,27 @@
 # under the License.
 import logging
 import uuid
-from typing import Any, Literal, Optional
+from typing import Any, Dict, Literal, Optional, Union


nit: we can now use dict and | in type annotations as long as we do the following import at the beginning:

from __future__ iimport annotations

Thank you for pointing that out. I'll make sure to update the code accordingly.

villebro · 2024-08-27T20:31:23Z

superset/async_events/async_query_manager.py

+        # Decode bytes to strings, decode_responses is not supported at RedisCache and RedisSentinelCache
+        if isinstance(self._cache, (RedisSentinelCacheBackend, RedisCacheBackend)):
+            decoded_results = [
+                (
+                    event_id.decode("utf-8"),
+                    {
+                        key.decode("utf-8"): value.decode("utf-8")
+                        for key, value in event_data.items()
+                    },
+                )
+                for event_id, event_data in results
+            ]
+            return (
+                [] if not decoded_results else list(map(parse_event, decoded_results))
+            )


I'm not sure this part was mentioned in the description. Can you elaborate on the need for this change?

RedisCache and RedisSentinelCache typically don't support built-in decoding like decode_responses. They return data in bytes, which needs to be manually decoded into the desired formats (e.g., strings).

https://github.com/pallets-eco/flask-caching/blob/master/src/flask_caching/backends/rediscache.py#L155

Got it, I hadn't noticed we were using redis.Redis, I thought we were using RedisCache from flask_caching here. Thanks for clarifying.

villebro · 2024-08-27T22:17:35Z

tests/unit_tests/async_events/async_query_manager_tests.py

+# Define the cache backends once as mocks
+cache_backends = {
+    "RedisCacheBackend": mock.Mock(spec=RedisCacheBackend),
+    "RedisSentinelCacheBackend": mock.Mock(spec=RedisSentinelCacheBackend),
+    "redis.Redis": mock.Mock(spec=redis.Redis),
+}
+


Could this loop be replaced by pytest.mark.parametrize? It feels more idiomatic (unless I'm missing some reason why it needs to be implemented like this..)

Yes, that's a great suggestion. I'll update the code with pytest.mark.parametrize and commit the changes.

Using pytest.mark.parametrize with mock from unittest caused compatibility issues. Switching to parameterized.expand resolved these issues and successfully covered the test cases, leading to a successful CI run. I’ll revisit this case later for further investigation.

@nsivarajan I believe it's generally suggested to use pytest_mock with pytest test cases. I don't want to drag out this review process more than necessary, but I think the cleanest solution would be one with pytest.mark.parametrize and pytest_mock. LMKWYT, but in the meantime let me re-review the functional parts.

Thanks, @villebro , for the review. I agree that using the standard approach is ideal. However, I've encountered continuous failures when combining pytest.mark.parametrize with mock, which suggests there might be compatibility issues. After some research, I found references that indicate potential conflicts when using pytest features with unittest.TestCase subclasses and mock (see pytest documentation, SeleniumBase issue, and pytest issue).

I noticed that our repository already uses parameterized.expand in similar cases, so I opted for it here to ensure the tests run smoothly.

Would it be possible to proceed with this PR using parameterized.expand? I'll continue investigating the issues with pytest.mark.parametrize and mock and plan to submit a follow-up PR if I find a viable solution.

Thanks for putting in the extra effort on these tests @nsivarajan ! Let's not let this derail your PR - I can also take a stab at migrating the remaining components to pytest (seems like a fun coffee break challenge).

villebro

A few minor observations during second review round

villebro · 2024-08-29T16:50:28Z

tests/integration_tests/tasks/async_queries_tests.py

+# Define the cache backends once
+cache_backends = {
+    "RedisCacheBackend": mock.Mock(spec=RedisCacheBackend),
+    "RedisSentinelCacheBackend": mock.Mock(spec=RedisSentinelCacheBackend),
+    "redis.Redis": mock.Mock(spec=redis.Redis),
+}
+


I believe these are now redundant as we have the parametrized tests?

Suggested change

# Define the cache backends once

cache_backends = {

"RedisCacheBackend": mock.Mock(spec=RedisCacheBackend),

"RedisSentinelCacheBackend": mock.Mock(spec=RedisSentinelCacheBackend),

"redis.Redis": mock.Mock(spec=redis.Redis),

}

# Define the cache backends once

cache_backends = {

"RedisCacheBackend": mock.Mock(spec=RedisCacheBackend),

"RedisSentinelCacheBackend": mock.Mock(spec=RedisSentinelCacheBackend),

"redis.Redis": mock.Mock(spec=redis.Redis),

}

Thanks, I'll get this removed.

Handled in last commit

villebro · 2024-08-29T16:50:59Z

tests/unit_tests/async_events/async_query_manager_tests.py

+# Define the cache backends once as mocks
+# cache_backends = {
+#    "RedisCacheBackend": mock.Mock(spec=RedisCacheBackend),
+#    "RedisSentinelCacheBackend": mock.Mock(spec=RedisSentinelCacheBackend),
+#    "redis.Redis": mock.Mock(spec=redis.Redis),
+# }
+


Same here:

Suggested change

# Define the cache backends once as mocks

# cache_backends = {

# "RedisCacheBackend": mock.Mock(spec=RedisCacheBackend),

# "RedisSentinelCacheBackend": mock.Mock(spec=RedisSentinelCacheBackend),

# "redis.Redis": mock.Mock(spec=redis.Redis),

# }

Thanks, I'll get this removed.

Updated in last commit

villebro

LGTM, thanks for all the hard work here! I've left a few extra type annotation tips, but they're not blocking (we've already done enough iterating here). I'll add a new task to our 5.0 project board to remove the old config. If you feel up for it, please open a PR with the change that removes the old config option so we can merge it when the 5.0 breaking window is opened up.

villebro · 2024-08-30T14:48:07Z

superset/async_events/async_query_manager.py

+
+def get_cache_backend(
+    config: dict[str, Any],
+) -> Union[RedisCacheBackend, RedisSentinelCacheBackend, redis.Redis]:  # type: ignore


nit: the new type annotations support this simpler format:

Suggested change

) -> Union[RedisCacheBackend, RedisSentinelCacheBackend, redis.Redis]: # type: ignore

) -> RedisCacheBackend | RedisSentinelCacheBackend | redis.Redis: # type: ignore

villebro · 2024-08-30T14:48:42Z

superset/async_events/async_query_manager.py

@@ -73,7 +103,7 @@ class AsyncQueryManager:

    def __init__(self) -> None:
        super().__init__()
-        self._redis: redis.Redis  # type: ignore
+        self._cache: Optional[BaseCache] = None


Same here:

Suggested change

self._cache: Optional[BaseCache] = None

self._cache: BaseCache | None = None

nsivarajan · 2024-08-30T18:15:52Z

Thanks, @villebro, for the feedback and type annotation tips!

I appreciate your guidance throughout this process. I'll go ahead and open a new PR to remove the old config option and will reference the task once it's created on the project board, so it’ll be ready when the 5.0 breaking window opens up. Looking forward to wrapping this up and moving on to the next steps!

villebro · 2024-08-30T19:31:28Z

Thanks @nsivarajan, I really appreciate the help! Here's the project card: https://github.com/orgs/apache/projects/345?pane=issue&itemId=78081192

Add Redis Sentinel Support for Global Async Queries

cc3cbd9

pull-request-size bot added the size/XL label Aug 10, 2024

nsivarajan changed the title ~~Feat (GAQ) : Add Redis Sentinel Support for Global Async Queries~~ feat (GAQ) : Add Redis Sentinel Support for Global Async Queries Aug 10, 2024

nsivarajan changed the title ~~feat (GAQ) : Add Redis Sentinel Support for Global Async Queries~~ feat : (GAQ) Add Redis Sentinel Support for Global Async Queries Aug 10, 2024

nsivarajan changed the title ~~feat : (GAQ) Add Redis Sentinel Support for Global Async Queries~~ feat(GAQ): Add Redis Sentinel Support for Global Async Queries Aug 10, 2024

Fix Workflow failure cases

fa85476

nsivarajan marked this pull request as ready for review August 10, 2024 20:39

dosubot bot added the global:async-query Related to Async Queries feature label Aug 10, 2024

villebro self-requested a review August 12, 2024 17:02

michael-s-molina requested a review from mistercrunch August 13, 2024 17:21

villebro reviewed Aug 27, 2024

View reviewed changes

Sivarajan Narayanan added 4 commits August 28, 2024 20:26

Handle Suggestions

e3779e6

Handle integration_tests failure

e4ff47c

Handle ruff_format failure

1537110

Handle testCase failure

23860ac

villebro reviewed Aug 29, 2024

View reviewed changes

Sivarajan Narayanan added 2 commits August 30, 2024 10:42

Handle testCase failure with pytest

7f068d7

Handle testCase failure with parameterized

4cc558a

pull-request-size bot added size/L and removed size/XL labels Aug 30, 2024

villebro approved these changes Aug 30, 2024

View reviewed changes

villebro merged commit 103cd3d into apache:master Aug 30, 2024
38 of 39 checks passed

nsivarajan mentioned this pull request Sep 15, 2024

chore(GAQ): [Merge 5.0] Breaking change: Remove GLOBAL_ASYNC_QUERIES_REDIS_CONFIG #30284

Open

9 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(GAQ): Add Redis Sentinel Support for Global Async Queries #29912

feat(GAQ): Add Redis Sentinel Support for Global Async Queries #29912

nsivarajan commented Aug 10, 2024

codecov bot commented Aug 10, 2024 •

edited

Loading

nsivarajan commented Aug 12, 2024

villebro commented Aug 12, 2024

nsivarajan commented Aug 15, 2024

nsivarajan commented Aug 22, 2024

rusackas commented Aug 22, 2024

villebro left a comment

villebro Aug 12, 2024 •

edited

Loading

nsivarajan Aug 28, 2024

villebro Aug 27, 2024

nsivarajan Aug 28, 2024

villebro Aug 29, 2024

villebro Aug 27, 2024

nsivarajan Aug 28, 2024

nsivarajan Aug 29, 2024

villebro Aug 29, 2024

nsivarajan Aug 30, 2024

villebro Aug 30, 2024

villebro left a comment

villebro Aug 29, 2024

nsivarajan Aug 30, 2024

nsivarajan Aug 30, 2024

villebro Aug 29, 2024

nsivarajan Aug 30, 2024

nsivarajan Aug 30, 2024

villebro left a comment

villebro Aug 30, 2024

villebro Aug 30, 2024

nsivarajan commented Aug 30, 2024

villebro commented Aug 30, 2024

	) -> Union[RedisCacheBackend, RedisSentinelCacheBackend, redis.Redis]: # type: ignore
	) -> RedisCacheBackend \| RedisSentinelCacheBackend \| redis.Redis: # type: ignore

	self._cache: Optional[BaseCache] = None
	self._cache: BaseCache \| None = None

feat(GAQ): Add Redis Sentinel Support for Global Async Queries #29912

feat(GAQ): Add Redis Sentinel Support for Global Async Queries #29912

Conversation

nsivarajan commented Aug 10, 2024

SUMMARY

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Aug 10, 2024 • edited Loading

Codecov Report

nsivarajan commented Aug 12, 2024

villebro commented Aug 12, 2024

nsivarajan commented Aug 15, 2024

nsivarajan commented Aug 22, 2024

rusackas commented Aug 22, 2024

villebro left a comment

Choose a reason for hiding this comment

villebro Aug 12, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

villebro left a 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

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsivarajan commented Aug 30, 2024

villebro commented Aug 30, 2024

codecov bot commented Aug 10, 2024 •

edited

Loading

villebro Aug 12, 2024 •

edited

Loading