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.

Sign up for GitHub

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

Jump to bottom

Fix GetMergeOperands in ReadOnlyDB and SecondaryDB #13340

Closed
wants to merge 4 commits into from
Closed

Fix GetMergeOperands in ReadOnlyDB and SecondaryDB #13340

wants to merge 4 commits into from

Conversation

jaykorean
Copy link
Contributor

@jaykorean jaykorean commented Jan 27, 2025 •
edited
Loading

Summary

Fixing the GetMergeOperands() in ReadOnlyDB and SecondaryDB as reported in #13243. Refactor in #11799 introduced this regression.

Follow ups to come

Test Plan

DBMergeOperandTest and DBSecondaryTest updated

./db_merge_operand_test --gtest_filter="*GetMergeOperandsBasic*"

./db_secondary_test -- --gtest_filter="*GetMergeOperands*"

@jaykorean jaykorean force-pushed the fix_get_merge_operands_readonly_db branch from 2457b1b to 248bdc5 Compare January 28, 2025 16:59
@jaykorean jaykorean marked this pull request as ready for review January 28, 2025 20:55
@jaykorean jaykorean requested review from cbi42 and ltamasi January 28, 2025 20:55
@facebook-github-bot
Copy link
Contributor

@jaykorean has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@jaykorean jaykorean requested a review from archang19 January 28, 2025 21:05
ltamasi
ltamasi reviewed Jan 28, 2025
View reviewed changes
db/db_merge_operand_test.cc
Comment on lines +312 to +314
ASSERT_OK(Merge("k6", "better"));
ASSERT_OK(Merge("k6", "call"));
ASSERT_OK(Merge("k6", "saul"));
Copy link
Contributor

Choose a reason for hiding this comment

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

LOL 😂

ltamasi
ltamasi approved these changes Jan 28, 2025
View reviewed changes
Copy link
Contributor

@ltamasi ltamasi left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@jaykorean jaykorean force-pushed the fix_get_merge_operands_readonly_db branch from fc23ade to 887660c Compare January 28, 2025 21:54
@facebook-github-bot
Copy link
Contributor

@jaykorean has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@jaykorean has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@jaykorean jaykorean force-pushed the fix_get_merge_operands_readonly_db branch from 887660c to bf27c8b Compare January 28, 2025 21:56
@facebook-github-bot
Copy link
Contributor

@jaykorean has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@jaykorean has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

archang19
archang19 reviewed Jan 28, 2025
View reviewed changes
Copy link
Contributor

@archang19 archang19 left a comment

Choose a reason for hiding this comment

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

I am not familiar with this part of the codebase, so just have a minor nit

db/db_secondary_test.cc Outdated

auto cfh = db_secondary_->DefaultColumnFamily();

// s.IsMergeInProgress()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I assume we want to g rid of this commented out line

@facebook-github-bot
Copy link
Contributor

@jaykorean has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@jaykorean has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@jaykorean merged this pull request in 2e0dc21.

@jaykorean jaykorean deleted the fix_get_merge_operands_readonly_db branch January 29, 2025 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@archang19 archang19 archang19 left review comments

@ltamasi ltamasi ltamasi approved these changes

@cbi42 cbi42 Awaiting requested review from cbi42

Assignees
No one assigned
Labels
CLA Signed Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jaykorean @facebook-github-bot @archang19 @ltamasi