Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(webscocket): helm support for async query with superset-websocket #21458

Closed
wants to merge 2 commits into from

Conversation

benjaminrene
Copy link

@benjaminrene benjaminrene commented Sep 13, 2022

SUMMARY

Requires #21379 to be resolved
Deploy superset-websocket on helm chart to enable QUERY_ASYNC feature for helm deployement.

Add a deployment dedicated to websocket
Add a service to expose websocket pods
Allow to configure a BASE_PATH on superset-websocket in order to expose websocket on the same port (80)
Add to the existing ingress a subpath to redirect websocket queries
Tune superset config to activate QUERY_ASYNC feature

TESTING INSTRUCTIONS

helm install with websocket.enabled = true on values file

ADDITIONAL INFORMATION

  • Has associated issue: Helm: deploy websocket for the benefit of query async feature #21457
  • 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

@Yann-J
Copy link
Contributor

Yann-J commented Oct 28, 2022

Hey FYI @benjaminrene I just noticed this PR, I think it's superseded by #21806 (which also includes other improvements such as more configurable probes and support for a flower UI).

It doesn't have some of the automatic Python config that your PR has... I think those are easy enough to pass separately as overrides, and I'm a bit cautious about forcing too many assumptions in the config - e.g. one may want to enabled websockets and set GLOBAL_ASYNC_QUERIES_TRANSPORT=ws in superset_config.py, but not use the Chart's deployment to run the websocket server... or craft their own GLOBAL_ASYNC_QUERIES_WEBSOCKET_URL that doesn't depend on the ingress settings (e.g. in my organization we don't use any chart's built-in ingresses because we maintain our own with a special security and approval process).

Unfortunately that PR was merged but the helm release process seems broken - #21963 will hopefully fix this.

I've also seen your PR #21379 which I'm also looking forward to! FYI you might be interested to look into #21954

@craig-rueda
Copy link
Member

Closing in favor of #21806

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants