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

Use rq_redis_connection when listing RQ jobs in remove_ghost_locks maintenance task #6539

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SchrosCat2013
Copy link
Contributor

What type of PR is this?

  • Refactor
  • Feature
  • Bug Fix
  • New Query Runner (Data Source)
  • New Alert Destination
  • Other

Description

Fix for UnicodeDecodeError in remove_ghost_locks mainentance task.

Redash has two connections to Redis - one for RQ, which does not support decode_responses in it's connection string, and one for Redash direct access to Redis, which expects decode_responses to be set. There is one place in the remove_ghost_locks call stack where the Redash connection is used in place of the RQ one, causing UnicodeDecodeErrors.

Discovered while investigating #6424

How is this tested?

  • Unit tests (pytest, jest)
  • E2E Tests (Cypress)
  • Manually
  • N/A
  • Modified the docker-compose file - set the worker command to worker instead of dev_worker (to make the healthcheck tasks run)
  • Started a long-running query (SELECT pg_sleep(300))
  • Before change - observe several UnicodeDecodeError: 'utf-8' codec can't decode byte 0x9c in position 1: invalid start byte in docker logs of redash-worker-1 during query execution
  • After change - query completes without UnicodeDecodeError in redash-worker-1 logs

@justinclift
Copy link
Member

This sounds logical to me, and seems pretty reasonable.

@guidopetri Assuming this passes our CI tests, are we ok to merge it? 😄

@guidopetri
Copy link
Collaborator

I don't think this change is correct. The only reference I see to rq_job_ids() is in redash/tasks/queries/maintenance.py, remove_ghost_locks(), which uses the jobs list returned by rq_job_ids to filter and delete locks present in redis_connection. So I think the function name must somehow be conflating what's going on. The title of this PR seems to be correct, but I don't think that the logic fix is correct.

@SchrosCat2013 can you verify if this function is being called correctly? Maybe look into the logging and see if the log line post-lock deletion is present + working as expected (i.e. actually removing locks that should be removed)?

Copy link
Collaborator

@guidopetri guidopetri left a comment

Choose a reason for hiding this comment

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

(placeholder)

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.

None yet

3 participants