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 memo in pinot #5902

Merged
merged 28 commits into from
Apr 18, 2024
Merged

Add memo in pinot #5902

merged 28 commits into from
Apr 18, 2024

Conversation

bowenxia
Copy link
Contributor

@bowenxia bowenxia commented Apr 11, 2024

What changed?
Add memo in pinot:
When write, add memo into search attribute
When read, get memo from search attribute and cleanup that field

NOTE: don't merge it until our customer test it locally

Why?
memo was stored in ES but can't be indexed(not in ES schema). We made the pinot schemas according to ES's schema, so we thought that we did't need memo in Pinot, but one customer was complaining about that.

How did you test it?
unit test
Also tested manually at local

Potential risks
not risk since Memo is not a functional attribute.

Release notes

Documentation Changes

Copy link

codecov bot commented Apr 11, 2024

Codecov Report

Merging #5902 (3c15b4e) into master (1620509) will increase coverage by 0.01%.
The diff coverage is 86.36%.

❗ Current head 3c15b4e differs from pull request most recent head 02a8cd1. Consider uploading reports for the commit 02a8cd1 to get more accurate results

Additional details and impacted files
Files Coverage Δ
common/pinot/response_utility.go 100.00% <100.00%> (ø)
common/persistence/pinot/pinot_visibility_store.go 93.23% <77.77%> (-0.32%) ⬇️
common/pinot/pinot_client.go 90.62% <42.85%> (-5.99%) ⬇️

... and 4 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 1620509...02a8cd1. Read the comment docs.

@coveralls
Copy link

coveralls commented Apr 11, 2024

Pull Request Test Coverage Report for Build 018eef8d-a474-4556-b940-6190a4e74bd7

Details

  • 48 of 54 (88.89%) changed or added relevant lines in 3 files are covered.
  • 59 unchanged lines in 16 files lost coverage.
  • Overall coverage decreased (-0.02%) to 67.538%

Changes Missing Coverage Covered Lines Changed/Added Lines %
common/persistence/pinot/pinot_visibility_store.go 9 11 81.82%
common/pinot/pinot_client.go 5 9 55.56%
Files with Coverage Reduction New Missed Lines %
service/history/task/transfer_standby_task_executor.go 2 87.84%
common/task/weighted_round_robin_task_scheduler.go 2 89.05%
service/history/shard/context.go 2 66.7%
common/task/fifo_task_scheduler.go 2 83.51%
common/persistence/sql/sqlplugin/mysql/task.go 2 73.68%
service/matching/db.go 2 73.23%
common/persistence/sql/sqlplugin/mysql/db.go 2 79.49%
common/log/tag/tags.go 3 50.46%
service/history/queue/timer_queue_processor_base.go 3 77.82%
service/history/queue/timer_gate.go 3 95.83%
Totals Coverage Status
Change from base Build 018eeeee-9517-449e-b456-d07d85466364: -0.02%
Covered Lines: 98878
Relevant Lines: 146404

💛 - Coveralls

@bowenxia bowenxia enabled auto-merge (squash) April 16, 2024 23:33
@bowenxia bowenxia disabled auto-merge April 17, 2024 19:40
common/pinot/response_utility.go Outdated Show resolved Hide resolved
@bowenxia bowenxia enabled auto-merge (squash) April 18, 2024 18:45
@bowenxia bowenxia merged commit 6564701 into master Apr 18, 2024
19 checks passed
@bowenxia bowenxia deleted the xbowen_hotfix_memo branch April 18, 2024 19:32
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