Skip to content
Snippets Groups Projects

Make ClickHouse sync write buffer compatible with Redis 6

Merged Felipe Cardozo requested to merge issue_506384_fix_lpop into master
All threads resolved!

What does this MR do and why?

Customers using Redis 6.0 have multiple errors in the logs related to ClickHouse sync workers. These are happening because when reading from the Redis buffer to write to ClickHouse we are using lpop with an extra limit argument which is not supported by Redis 6.

This MR removes the extra argument and instead uses a Redis pipeline to read the records in batches.

Customers with old redis versions should stop seeing error logs from UsageEvents::DumpWriteBufferCronWorker.

References

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Screenshots or screen recordings

No changes to the current behavior.

How to set up and validate locally

  1. On your local environment downgrade the Redis version to 6.0. If you are using asdf this can be achieved by:
    1. Change gdk .tool_versions of redis to 6.0.0
    2. Change gitlab folder .tool_versions redis to 6.0.0
    3. Rename or delete PATH_TO_GDK/redis/dump.rdb so redis 6 does not try to load the db from the newer version
    4. run asdf_install.
    5. OPTIONAL run asdf current and check if redis version is set to 6.0.0 on gitlab folder.
  2. Running rspec spec/lib/click_house/write_buffer_spec.rb will raise failures related to compatibility problems. Another way to reproduce this error is opening a rails console and running ClickHouse::WriteBuffer.pop('anything', 1)
  3. Checkout to this branch and repeat step 2, no errors should happen.
Edited by Felipe Cardozo

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Felipe Cardozo added 1 commit

    added 1 commit

    • 6c7fccb2 - Add derisk feature flag toggle

    Compare with previous version

  • Felipe Cardozo requested review from @pshutsin

    requested review from @pshutsin

  • A deleted user added feature flag label

    added feature flag label

  • Felipe Cardozo added 1 commit

    added 1 commit

    Compare with previous version

  • Felipe Cardozo requested review from @pshutsin

    requested review from @pshutsin

  • 2 Warnings
    :warning: This MR changes code in ee/, but its Changelog commit is missing the EE: true trailer. Consider adding it to your Changelog commits.
    :warning: 6c7fccb2: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines.

    Reviewer roulette

    Category Reviewer Maintainer
    backend @tvellishetty profile link current availability (UTC+5.5, 8.5 hours ahead of author) @jessieay profile link current availability (UTC-8, 5 hours behind author)
    ~"Clickhouse" Reviewer review is optional for ~"Clickhouse" @harsimarsandhu profile link current availability (UTC+5.5, 8.5 hours ahead of author)

    Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.

    If needed, you can retry the :repeat: danger-review job that generated this comment.

    Generated by :no_entry_sign: Danger

    Edited by ****
  • Pavel Shutsin resolved all threads

    resolved all threads

  • Pavel Shutsin approved this merge request

    approved this merge request

  • Pavel Shutsin enabled automatic add to merge train when checks pass

    enabled automatic add to merge train when checks pass

  • added pipelinetier-2 label and removed pipelinetier-1 label

  • Before you set this MR to auto-merge

    This merge request will progress on pipeline tiers until it reaches the last tier: pipelinetier-3. We will trigger a new pipeline for each transition to a higher tier.

    Before you set this MR to auto-merge, please check the following:

    • You are the last maintainer of this merge request
    • The latest pipeline for this merge request is pipelinetier-3 (You can find which tier it is in the pipeline name)
    • This pipeline is recent enough (created in the last 8 hours)

    If all the criteria above apply, please set auto-merge for this merge request.

    See pipeline tiers and merging a merge request for more details.

  • E2E Test Result Summary

    allure-report-publisher generated test report!

    e2e-test-on-gdk: :white_check_mark: test report for e439f768

    expand test summary
    +------------------------------------------------------------------+
    |                          suites summary                          |
    +-------------+--------+--------+---------+-------+-------+--------+
    |             | passed | failed | skipped | flaky | total | result |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Create      | 270    | 0      | 40      | 0     | 310   | ✅     |
    | Monitor     | 16     | 0      | 24      | 0     | 40    | ✅     |
    | Plan        | 164    | 0      | 16      | 0     | 180   | ✅     |
    | Release     | 10     | 0      | 2       | 0     | 12    | ✅     |
    | Verify      | 96     | 0      | 40      | 0     | 136   | ✅     |
    | Package     | 50     | 0      | 26      | 0     | 76    | ✅     |
    | Govern      | 160    | 0      | 24      | 0     | 184   | ✅     |
    | Data Stores | 66     | 0      | 20      | 0     | 86    | ✅     |
    | Manage      | 2      | 0      | 18      | 0     | 20    | ✅     |
    | Fulfillment | 4      | 0      | 14      | 0     | 18    | ✅     |
    | Secure      | 8      | 0      | 6       | 0     | 14    | ✅     |
    | ModelOps    | 0      | 0      | 2       | 0     | 2     | ➖     |
    | Analytics   | 4      | 0      | 0       | 0     | 4     | ✅     |
    | Configure   | 0      | 0      | 6       | 0     | 6     | ➖     |
    | Growth      | 0      | 0      | 4       | 0     | 4     | ➖     |
    | Ai-powered  | 0      | 0      | 4       | 0     | 4     | ➖     |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Total       | 850    | 0      | 246     | 0     | 1096  | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+

    e2e-test-on-cng: :white_check_mark: test report for e439f768

    expand test summary
    +------------------------------------------------------------------+
    |                          suites summary                          |
    +-------------+--------+--------+---------+-------+-------+--------+
    |             | passed | failed | skipped | flaky | total | result |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Monitor     | 8      | 0      | 12      | 0     | 20    | ✅     |
    | Plan        | 86     | 0      | 8       | 0     | 94    | ✅     |
    | Govern      | 84     | 0      | 10      | 0     | 94    | ✅     |
    | Create      | 140    | 0      | 19      | 0     | 159   | ✅     |
    | Secure      | 2      | 0      | 5       | 0     | 7     | ✅     |
    | Verify      | 49     | 0      | 19      | 0     | 68    | ✅     |
    | Data Stores | 33     | 0      | 10      | 0     | 43    | ✅     |
    | Package     | 30     | 0      | 14      | 0     | 44    | ✅     |
    | Analytics   | 2      | 0      | 0       | 0     | 2     | ✅     |
    | Release     | 5      | 0      | 1       | 0     | 6     | ✅     |
    | ModelOps    | 0      | 0      | 1       | 0     | 1     | ➖     |
    | Configure   | 0      | 0      | 3       | 0     | 3     | ➖     |
    | Manage      | 1      | 0      | 9       | 0     | 10    | ✅     |
    | Fulfillment | 2      | 0      | 7       | 0     | 9     | ✅     |
    | Ai-powered  | 0      | 0      | 2       | 0     | 2     | ➖     |
    | Growth      | 0      | 0      | 2       | 0     | 2     | ➖     |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Total       | 442    | 0      | 122     | 0     | 564   | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+
    Edited by ****
  • requested review from @pedropombeiro

  • Pavel Shutsin removed review request for @pshutsin

    removed review request for @pshutsin

  • Pedro Pombeiro approved this merge request

    approved this merge request

  • Pedro Pombeiro resolved all threads

    resolved all threads

  • merged

  • Pavel Shutsin mentioned in commit 61034803

    mentioned in commit 61034803

  • added workflowstaging label and removed workflowcanary label

  • Please register or sign in to reply
    Loading