Skip to content

Conversation

@snickerjp
Copy link
Member

@snickerjp snickerjp commented Nov 17, 2025

What type of PR is this?

  • Refactor
  • Bug Fix

Description

This PR implements an alternative approach to #7572 by catching NotSupported exceptions in refresh_schema() instead of pre-filtering datasource types.

Addresses issue #7571 - Clean up error logs from datasources that don't support schema refresh.

Motivation

As suggested by @yoshiokatsuneo in PR #7572 review, this approach is simpler and more maintainable:

  • No global configuration needed
  • Automatically handles all unsupported datasources
  • Cleaner separation of concerns

Changes

  • Add NotSupported exception handling to refresh_schema()
  • Log unsupported datasources at DEBUG level
  • Avoid incrementing error metrics for datasources without schema support

Files changed:

  • redash/tasks/queries/maintenance.py: +3 lines (1 import, 2 exception handling)
  • tests/tasks/test_refresh_schemas.py: +10 lines (1 test case)

Flow Diagram

flowchart TD
    Start([refresh_schemas]) --> Loop{For each<br/>datasource}
    Loop -->|Paused| Skip1[Skip: paused]
    Loop -->|Active| Queue[Enqueue refresh_schema job]
    
    Queue --> Worker[Worker executes job]
    Worker --> Try[Try: ds.get_schema refresh=True]
    
    Try -->|Success| Success[Log: state=finished<br/>Increment: success metric]
    Try -->|JobTimeoutException| Timeout[Log: state=timeout<br/>Increment: timeout metric]
    Try -->|NotSupported| NotSupp[Log DEBUG: not supported<br/>No metric increment]
    Try -->|Other Exception| Error[Log WARNING: failed<br/>Increment: error metric]
    
    Success --> JobOK1[Job OK]
    Timeout --> JobOK2[Job OK]
    NotSupp --> JobOK3[Job OK]
    Error --> JobOK4[Job OK]
    
    Skip1 --> Loop
    JobOK1 --> Loop
    JobOK2 --> Loop
    JobOK3 --> Loop
    JobOK4 --> Loop
    
    Loop -->|Done| End([End])
    
    style NotSupp fill:#2d5016,stroke:#4a7c2c,color:#fff
    style Success fill:#1e4d7b,stroke:#2e6da4,color:#fff
    style Error fill:#8b2e3b,stroke:#c9302c,color:#fff
    style Timeout fill:#8b6914,stroke:#d9a441,color:#fff
Loading

Key Points:

  • 🟢 NotSupported: Silently handled at DEBUG level, no error metrics
  • 🔵 Success: Normal completion with metrics
  • 🔴 Error: Other exceptions logged as warnings with metrics
  • 🟡 Timeout: Job timeout logged with metrics

How is this tested?

  • Unit tests (pytest, jest)
  • Manually

Unit Tests

$ docker compose run --rm server pytest tests/tasks/test_refresh_schemas.py -v
========================== test session starts ===========================
collected 3 items

tests/tasks/test_refresh_schemas.py::TestRefreshSchemas::test_calls_refresh_of_all_data_sources PASSED [ 33%]
tests/tasks/test_refresh_schemas.py::TestRefreshSchemas::test_skips_paused_data_sources PASSED [ 66%]
tests/tasks/test_refresh_schemas.py::TestRefreshSchemas::test_handles_notsupported_exception PASSED [100%]

========================== 3 passed in 8.41s =============================

Manual Testing

Tested with 3 datasources:

  • Query Results (type=results, ds_id=1) - NotSupported exception caught ✅
  • Python (type=python, ds_id=2) - NotSupported exception caught ✅
  • PostgreSQL (type=pg, ds_id=3) - Schema refresh succeeded ✅

Worker logs (INFO level):

[INFO] task=refresh_schema state=start ds_id=1
[INFO] schemas: Job OK (...)
[INFO] task=refresh_schema state=start ds_id=2
[INFO] schemas: Job OK (...)
[INFO] task=refresh_schema state=start ds_id=3
[INFO] task=refresh_schema state=finished ds_id=3 runtime=0.02
[INFO] schemas: Job OK (...)

No error logs, all jobs completed successfully.

DEBUG Log Verification

When DEBUG level is enabled, unsupported datasources are logged:

[DEBUG] Datasource Query Results does not support schema refresh

Comparison with PR #7572

Aspect This PR (Exception Handling) PR #7572 (Type Exclusion)
Configuration None required REDASH_SCHEMAS_REFRESH_EXCLUDED_TYPES
Code changes 3 lines 7 lines
Maintenance Automatic Manual type list
Job execution All datasources Filtered datasources
Flexibility Handles all NotSupported Only configured types

Benefits

  1. Simpler: No configuration needed
  2. Automatic: Works for all datasources that raise NotSupported
  3. Future-proof: New datasources automatically handled
  4. Clean logs: No error messages in production (DEBUG level only)

Log Output Behavior

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

N/A - Backend changes only

- Add NotSupported exception handling to refresh_schema()
- Log unsupported datasources at DEBUG level
- Avoid error metrics for datasources without schema support
- Test that NotSupported exceptions are caught and logged at DEBUG level
- Verify no warning logs are generated for unsupported datasources
@yoshiokatsuneo
Copy link
Contributor

yoshiokatsuneo commented Nov 18, 2025

@snickerjp

Thank you for your PR!

I just feel that the test is too much as the test is testing only logging for this 3 lines of code.
I think, If we add tests for every logging like this in whole redash code base, I feel it does not improve the code quality but may rather make it difficult to read or maintain the whole code or tests.

How about ?

@yoshiokatsuneo
Copy link
Contributor

@snickerjp

And, this is just my curious. And, I'm sorry if it is my mistaken.
May I ask whether you are using AI agent to create the PR, code, and/or tests ?
(Again, I'm not against it(I also sometimes uses AI), but it may make it easy to understand the context as AI agents sometimes makes more code than the human.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants