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: Containerize WebSocket server #14514

Merged
merged 3 commits into from
May 10, 2021

Conversation

benjreinhart
Copy link
Contributor

SUMMARY

This adds Docker and docker-compose support for the websocket server.

Notes

Since we support development both with and without Docker, we need to make sure that any node_modules and dist folders/files from the host machine are not copied into the container, those should be freshly installed/compiled. .dockerignore ensures that COPY commands in the Dockerfile will not carry those folders over when building the image with docker build. However, this did not seem to work when running the service with docker-compose. Instead, it works to mount fake volumes for those folders in the compose file. Since the compose file is for development, I think it's ok to go with this somewhat hacky solution (it seems to work fine).

Try it out

For example, if you want to run superset via docker with async queries via websockets, you can:

  1. Ensure you properly configure async queries (and whatever else) in docker/pythonpath_dev/superset_config_docker.py
  2. Boot relevant superset services with docker-compose, e.g.,
     docker-compose up superset superset-websocket superset-worker
    

cc @robdiciuccio @stevenuray

# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
FROM node:14.16.1
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the most recent LTS version, which is why I chose this. However, happy to change it to another version if folks are opinionated here.

@villebro
Copy link
Member

villebro commented May 7, 2021

This is awesome, thanks @benjreinhart !

@robdiciuccio
Copy link
Member

@benjreinhart how is the websocket app configured in the docker-compose workflow? For example, how would I set the jwtSecret value to match what's configured in docker/pythonpath_dev/superset_config_docker.py?

@robdiciuccio
Copy link
Member

robdiciuccio commented May 7, 2021

Seeing consistent redis connection errors when running this locally:

superset_websocket       | [ioredis] Unhandled error event: Error: connect ETIMEDOUT
superset_websocket       |     at TLSSocket.<anonymous> (/home/superset-websocket/node_modules/ioredis/built/redis/index.js:283:31)
superset_websocket       |     at Object.onceWrapper (events.js:421:28)
superset_websocket       |     at TLSSocket.emit (events.js:315:20)
superset_websocket       |     at TLSSocket.Socket._onTimeout (net.js:483:8)
superset_websocket       |     at listOnTimeout (internal/timers.js:554:17)
superset_websocket       |     at processTimers (internal/timers.js:497:7)

But the http://127.0.0.1:8080/health endpoint still returns "OK"

@benjreinhart
Copy link
Contributor Author

@robdiciuccio it should be the same as non-docker workflow, i.e., adding it to the config.json file.

The redis issue is interesting. docker-compose should be overriding a few values from the config.json file, for example, pointing the redis host to redis (the docker-compose service running redis).

Copy link
Member

@robdiciuccio robdiciuccio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The redis connection issue was with ssl: true in my config.json, which has now been addressed.

@robdiciuccio robdiciuccio merged commit f33c433 into apache:master May 10, 2021
@robdiciuccio robdiciuccio deleted the benjreinhart/ws-container branch May 10, 2021 16:28
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
* feat: Containerize WebSocket server

* Add license

* Ensure Redis SSL is always turned off in dev
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
* feat: Containerize WebSocket server

* Add license

* Ensure Redis SSL is always turned off in dev
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
* feat: Containerize WebSocket server

* Add license

* Ensure Redis SSL is always turned off in dev
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants