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

Add double read for latency comparison for Pinot Migration #5927

Merged
merged 8 commits into from
Apr 24, 2024

Conversation

bowenxia
Copy link
Contributor

@bowenxia bowenxia commented Apr 23, 2024

What changed?
Added a double-read flipr config in pinot_visibility_triple_manager
Using go routines to avoid breaking the normal read flow and log the shadow read errors

Why?
We need this to do a latency comparison for Pinot Migration. And we don't want the secondary read to influence the main data flow, thus we need to use concurrency to make the 2 read actions run in the same time.

How did you test it?
Unit test

Potential risks
null

Release notes

Documentation Changes

Copy link

codecov bot commented Apr 23, 2024

Codecov Report

Attention: Patch coverage is 98.21429% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 62.56%. Comparing base (882796c) to head (b4a09b3).

Additional details and impacted files
Files Coverage Δ
...mon/persistence/pinot_visibility_triple_manager.go 100.00% <100.00%> (ø)
common/persistence/client/factory.go 53.18% <0.00%> (-0.20%) ⬇️

... and 6 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 882796c...b4a09b3. Read the comment docs.

@coveralls
Copy link

coveralls commented Apr 23, 2024

Pull Request Test Coverage Report for Build 018f1252-12af-42a6-8315-71626d05973c

Details

  • 111 of 112 (99.11%) changed or added relevant lines in 4 files are covered.
  • 62 unchanged lines in 13 files lost coverage.
  • Overall coverage increased (+0.03%) to 67.742%

Changes Missing Coverage Covered Lines Changed/Added Lines %
common/persistence/client/factory.go 0 1 0.0%
Files with Coverage Reduction New Missed Lines %
common/task/weighted_round_robin_task_scheduler.go 1 89.05%
service/history/task/transfer_standby_task_executor.go 2 88.27%
service/matching/taskReader.go 2 84.88%
common/persistence/execution_manager.go 2 83.74%
service/history/task/transfer_active_task_executor.go 2 72.85%
service/history/queue/transfer_queue_processor.go 2 57.17%
common/persistence/statsComputer.go 2 97.14%
service/history/task/fetcher.go 2 85.57%
host/taskpoller.go 3 71.94%
common/task/fifo_task_scheduler.go 3 84.54%
Totals Coverage Status
Change from base Build 018f1248-bf4e-4f7f-ada2-77eea52238e3: 0.03%
Covered Lines: 99364
Relevant Lines: 146680

💛 - Coveralls

Copy link
Member

@davidporter-id-au davidporter-id-au left a comment

Choose a reason for hiding this comment

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

Just need to understand one piece of intent, rest looks good

common/persistence/pinot_visibility_triple_manager.go Outdated Show resolved Hide resolved
@bowenxia bowenxia merged commit 0b41007 into master Apr 24, 2024
21 checks passed
@bowenxia bowenxia deleted the xbowen_add_double_read00 branch April 24, 2024 23:40
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.

4 participants