You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "villebro (via GitHub)" <gi...@apache.org> on 2026/10/17 02:56:18 UTC

[PR] fix(coordination): honor CACHE_REDIS_DB and CACHE_REDIS_USER in Sentinel backend [superset]

villebro opened a new pull request, #45402:
URL: https://github.com/apache/superset/pull/45402

   ### SUMMARY
   When `DISTRIBUTED_COORDINATION_CONFIG` uses `RedisSentinelCache`, `RedisSentinelCacheBackend` runs all coordination commands (GTF state, locks) through the connection returned by `Sentinel.master_for(...)`. That connection was built without `db` or `username`: `db` was only passed to the flask-caching parent constructor, whose clients the coordination mixin does not use, and `from_config` never read `CACHE_REDIS_USER`. As a result, coordination keys landed in DB 0 regardless of `CACHE_REDIS_DB`, and an ACL username could not be used. Pub/sub was unaffected since it is not scoped by DB.
   
   This passes `db` to `master_for`, adds a `username` parameter forwarded when set, and reads `CACHE_REDIS_USER` in `RedisSentinelCacheBackend.from_config`, mirroring `RedisCacheBackend.from_config`.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   N/A
   
   ### TESTING INSTRUCTIONS
   Run `pytest tests/unit_tests/coordination/test_cache_backend.py`. The new test asserts that `from_config` with `CACHE_REDIS_DB=7` and `CACHE_REDIS_USER="u"` produces a master connection with `db=7` and `username="u"`. For manual verification, point coordination at a Sentinel deployment with `CACHE_REDIS_DB=7` and confirm coordination keys appear in DB 7, and that an ACL user restricted to DB 7 keys works.
   
   ### ADDITIONAL INFORMATION
   - [ ] Has associated issue:
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] 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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(coordination): honor CACHE_REDIS_DB and CACHE_REDIS_USER in Sentinel backend [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45402:
URL: https://github.com/apache/superset/pull/45402#discussion_r4301023769


##########
superset/coordination/cache_backend.py:
##########
@@ -408,6 +409,12 @@ def __init__(  # pylint: disable=too-many-arguments
         master_kwargs["socket_connect_timeout"] = socket_connect_timeout
         master_kwargs["protocol"] = 2
 
+        # The coordination commands run on this master connection rather than
+        # on the flask-caching client, so the DB and ACL user must be set here.
+        master_kwargs["db"] = db
+        if username:
+            master_kwargs["username"] = username

Review Comment:
   **Suggestion:** `CACHE_REDIS_USER` reaches only the master connection; if Sentinel nodes require ACL authentication, discovery omits the username and coordination initialization fails.
   
   **Assessment:** ๐ŸŸ  `Major` ยท ๐Ÿ” `Occurrence: Sometimes` ยท ๐Ÿท๏ธ `Api mismatch`
   
   [![Use CodeAnt Skill](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/use-codeant-skill-flat-v2.svg)](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [![Fix in Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0a6385bc9dfd4d91aeb18a5b0d3529cf&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [![Fix in VSCode Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0a6385bc9dfd4d91aeb18a5b0d3529cf&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   <details>
   <summary><b>Prompt for AI Agent ๐Ÿค– </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** superset/coordination/cache_backend.py
   **Line:** 415:416
   **Comment:**
   	*Api Mismatch: `CACHE_REDIS_USER` reaches only the master connection; if Sentinel nodes require ACL authentication, discovery omits the username and coordination initialization fails.
   
   Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
   Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
   ```
   </details>
   <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45402&comment_hash=d44215847e2c32202dd1a9a32db1e58ac69152aa088b3fa19d9c66994827e78d&reaction=like'>๐Ÿ‘</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45402&comment_hash=d44215847e2c32202dd1a9a32db1e58ac69152aa088b3fa19d9c66994827e78d&reaction=dislike'>๐Ÿ‘Ž</a>



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(coordination): honor CACHE_REDIS_DB and CACHE_REDIS_USER in Sentinel backend [superset]

Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #45402:
URL: https://github.com/apache/superset/pull/45402#discussion_r4308053038


##########
superset/coordination/cache_backend.py:
##########
@@ -408,6 +409,12 @@ def __init__(  # pylint: disable=too-many-arguments
         master_kwargs["socket_connect_timeout"] = socket_connect_timeout
         master_kwargs["protocol"] = 2
 
+        # The coordination commands run on this master connection rather than
+        # on the flask-caching client, so the DB and ACL user must be set here.
+        master_kwargs["db"] = db
+        if username:
+            master_kwargs["username"] = username

Review Comment:
   Not changing this. The existing code already treats Sentinel-node auth and data-node auth as separate credentials: `sentinel_kwargs` carries `sentinel_password` (from `CACHE_REDIS_SENTINEL_PASSWORD`), while `password` goes only to `master_for`. `CACHE_REDIS_USER` is the ACL user for the data (master) connection, paired with `CACHE_REDIS_PASSWORD`, so forwarding it to the Sentinel nodes would pair it with the wrong password and could break discovery for deployments where the data user does not exist on the Sentinels. A Sentinel-node username would need its own config key (e.g. a sentinel-specific counterpart), which is out of scope for this fix and not an existing option.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(coordination): honor CACHE_REDIS_DB and CACHE_REDIS_USER in Sentinel backend [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45402:
URL: https://github.com/apache/superset/pull/45402#discussion_r4308055206


##########
superset/coordination/cache_backend.py:
##########
@@ -408,6 +409,12 @@ def __init__(  # pylint: disable=too-many-arguments
         master_kwargs["socket_connect_timeout"] = socket_connect_timeout
         master_kwargs["protocol"] = 2
 
+        # The coordination commands run on this master connection rather than
+        # on the flask-caching client, so the DB and ACL user must be set here.
+        master_kwargs["db"] = db
+        if username:
+            master_kwargs["username"] = username

Review Comment:
   โœ… **Customized review instruction saved!**
   
   **Instruction:**
   > Keep Sentinel-node authentication separate from data-node ACL credentials; do not forward `CACHE_REDIS_USER` to Sentinel nodes. Sentinel ACL authentication requires a separate Sentinel-specific configuration option.
   
   **Applied to:**
     - `superset/coordination/cache_backend.py`
   
   ---
   ๐Ÿ’ก *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(coordination): honor CACHE_REDIS_DB and CACHE_REDIS_USER in Sentinel backend [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #45402:
URL: https://github.com/apache/superset/pull/45402#issuecomment-6228828775

   ## CodeAnt PR Risk: Medium Risk
   
   - The PR needs attention before merging.
   - CACHE_REDIS_USER is forwarded to the master connection, but not Sentinel discovery, so ACL-protected Sentinel nodes may still prevent coordination setup.
   - The new tests verify the master connection settings but do not cover authentication to Sentinel nodes.
   
   **Assessed commit:** `d3075d31fdd8`


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(coordination): honor CACHE_REDIS_DB and CACHE_REDIS_USER in Sentinel backend [superset]

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #45402:
URL: https://github.com/apache/superset/pull/45402#issuecomment-6228911945

   ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/45402?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :x: Patch coverage is `0%` with `5 lines` in your changes missing coverage. Please review.
   :white_check_mark: Project coverage is 64.92%. Comparing base ([`a5d29a9`](https://app.codecov.io/gh/apache/superset/commit/a5d29a9d27d2951e203ce65174d1af7cb5443d2f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`d3075d3`](https://app.codecov.io/gh/apache/superset/commit/d3075d31fdd84f5a26e5be808cf2ddd2e2064cfc?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/45402?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [superset/coordination/cache\_backend.py](https://app.codecov.io/gh/apache/superset/pull/45402?src=pr&el=tree&filepath=superset%2Fcoordination%2Fcache_backend.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29vcmRpbmF0aW9uL2NhY2hlX2JhY2tlbmQucHk=) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/45402?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #45402      +/-   ##
   ==========================================
   - Coverage   65.15%   64.92%   -0.24%     
   ==========================================
     Files        3048     3048              
     Lines      199619   199624       +5     
     Branches    46578    46580       +2     
   ==========================================
   - Hits       130063   129602     -461     
   - Misses      66629    67091     +462     
   - Partials     2927     2931       +4     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/superset/pull/45402/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage ฮ” | |
   |---|---|---|
   | [hive](https://app.codecov.io/gh/apache/superset/pull/45402/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.98% <0.00%> (-0.01%)` | :arrow_down: |
   | [mysql](https://app.codecov.io/gh/apache/superset/pull/45402/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [postgres](https://app.codecov.io/gh/apache/superset/pull/45402/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [presto](https://app.codecov.io/gh/apache/superset/pull/45402/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `36.73% <0.00%> (-0.01%)` | :arrow_down: |
   | [python](https://app.codecov.io/gh/apache/superset/pull/45402/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `52.81% <0.00%> (-0.42%)` | :arrow_down: |
   | [sqlite](https://app.codecov.io/gh/apache/superset/pull/45402/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `52.70% <0.00%> (-0.01%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   </details>
   
   [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/45402?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org