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

Multiget LDB Followup #12332

Closed
wants to merge 1 commit into from
Closed

Multiget LDB Followup #12332

wants to merge 1 commit into from

Conversation

jaykorean
Copy link
Contributor

@jaykorean jaykorean commented Feb 5, 2024 •
edited
Loading

Summary

Following up @jowlyzhang 's comment in #12283 .

  • Remove ARG_TTL from help which is not relevant to multi_get command
  • Treat NotFound status as non-error case for both Get and MultiGet and updated the unit test, ldb_test.py
  • Print key along with value in multi_get command

Test Plan

Unit Test

$>python3 tools/ldb_test.py
...
Ran 25 tests in 17.447s

OK

Manual Run

$> ./ldb --db=/data/users/jewoongh/rocksdb_test/T173992396/rocksdb_crashtest_blackbox --hex multi_get 0x0000000000000009000000000000012B00000000000000D8 0x0000000000000009000000000000002678787878BEEF
0x0000000000000009000000000000012B00000000000000D8 ==> 0x47000000434241404F4E4D4C4B4A494857565554535251505F5E5D5C5B5A595867666564636261606F6E6D6C6B6A696877767574737271707F7E7D7C7B7A797807060504030201000F0E0D0C0B0A090817161514131211101F1E1D1C1B1A1918
Key not found: 0x0000000000000009000000000000002678787878BEEF

$> ./ldb --db=/data/users/jewoongh/rocksdb_test/T173992396/rocksdb_crashtest_blackbox --hex get 0x00000000000000090000000000
Key not found

@jaykorean jaykorean requested review from jowlyzhang and hx235 February 5, 2024 23:04
@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.

jowlyzhang
jowlyzhang approved these changes Feb 6, 2024
View reviewed changes
Copy link
Contributor

@jowlyzhang jowlyzhang left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

tools/ldb_cmd.cc
values[i].ToString(is_value_hex_).c_str());
} else if (statuses[i].IsNotFound()) {
fprintf(stdout, "Key not found: %s\n",
(is_key_hex_ ? StringToHex(keys_[i]) : keys_[i]).c_str());
Copy link
Contributor

Choose a reason for hiding this comment

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

In the below else block, the failed flag is supposed to be set to true, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. If you expand the lines hidden by git, It's there (in line 2942)

@jaykorean jaykorean force-pushed the multiget_in_ldb_followup branch from 7ee31ed to 8268c90 Compare February 6, 2024 00:12
@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 0088f77.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jowlyzhang jowlyzhang jowlyzhang approved these changes

@hx235 hx235 Awaiting requested review from hx235

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

Successfully merging this pull request may close these issues.
3 participants
@jaykorean @facebook-github-bot @jowlyzhang