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 support in ldb #12283

Closed
wants to merge 1 commit into from
Closed

MultiGet support in ldb #12283

wants to merge 1 commit into from

Conversation

jaykorean
Copy link
Contributor

Summary

While investigating test failures due to the inconsistency between Get() and MultiGet(), I realized that LDB currently doesn't support MultiGet(). This PR introduces the MultiGet() support in LDB. Tested the command manually. Unit test will follow in a separate PR.

Test Plan

When key not found

$> ./ldb --db=/data/users/jewoongh/rocksdb_test/T173992396/rocksdb_crashtest_blackbox --hex multi_get 0x0000000000000009000000000000012B00000000000002AB                                                                                                                                                                                                                                                           
Status for key 0x0000000000000009000000000000012B00000000000002AB: NotFound:

Compare the same key with get

$> ./ldb --db=/data/users/jewoongh/rocksdb_test/T173992396/rocksdb_crashtest_blackbox --hex get 0x0000000000000009000000000000012B00000000000002AB
Failed: Get failed: NotFound:

Multiple keys not found

$> ./ldb --db=/data/users/jewoongh/rocksdb_test/T173992396/rocksdb_crashtest_blackbox --hex multi_get 0x0000000000000009000000000000012B00000000000002AB 0x0000000000000009000000000000012B00000000000002AC                                                                                                                                                                                                        Status for key 0x0000000000000009000000000000012B00000000000002AB: NotFound: 
Status for key 0x0000000000000009000000000000012B00000000000002AC: NotFound:

One of the keys found

$> ./ldb --db=/data/users/jewoongh/rocksdb_test/T173992396/rocksdb_crashtest_blackbox --hex multi_get 0x0000000000000009000000000000012B00000000000002AB 0x00000000000000090000000000000026787878787878                                                                                              
Status for key 0x0000000000000009000000000000012B00000000000002AB: NotFound: 
0x22000000262724252A2B28292E2F2C2D32333031363734353A3B38393E3F3C3D02030001060704050A0B08090E0F0C0D12131011161714151A1B18191E1F1C1D

All of the keys found

$> ./ldb --db=/data/users/jewoongh/rocksdb_test/T173992396/rocksdb_crashtest_blackbox --hex multi_get 0x0000000000000009000000000000012B00000000000000D8 0x00000000000000090000000000000026787878787878                                                                                                                                                                                                            15:57:03
0x47000000434241404F4E4D4C4B4A494857565554535251505F5E5D5C5B5A595867666564636261606F6E6D6C6B6A696877767574737271707F7E7D7C7B7A797807060504030201000F0E0D0C0B0A090817161514131211101F1E1D1C1B1A1918
0x22000000262724252A2B28292E2F2C2D32333031363734353A3B38393E3F3C3D02030001060704050A0B08090E0F0C0D12131011161714151A1B18191E1F1C1D

@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.

hx235
hx235 approved these changes Jan 24, 2024
View reviewed changes
Copy link
Contributor

@hx235 hx235 left a comment

Choose a reason for hiding this comment

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

LGTM

jowlyzhang
jowlyzhang reviewed Jan 24, 2024
View reviewed changes
tools/ldb_cmd.cc
ret.append(" ");
ret.append(MultiGetCommand::Name());
ret.append(" <key_1> <key_2> <key_3> ...");
ret.append(" [--" + ARG_TTL + "]");
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: is this supported for multi_get?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, no. It's from copy-pasta. I can clean this up.

jowlyzhang
jowlyzhang reviewed Jan 24, 2024
View reviewed changes
tools/ldb_cmd.cc
fprintf(stderr, "Status for key %s: %s\n",
(is_key_hex_ ? StringToHex(keys_[i]) : keys_[i]).c_str(),
statuses[i].ToString().c_str());
failed = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intended to mark failed as true? I think we should isolate NotFound separately from other non OK status since it's strictly not a failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I agree with your suggestion, I tried to be consistent with what Get() does first. I think we can fix both commands to not consider NotFound as failed command.

jowlyzhang
jowlyzhang reviewed Jan 24, 2024
View reviewed changes
tools/ldb_cmd.cc
bool failed = false;
for (size_t i = 0; i < num_keys; ++i) {
if (statuses[i].ok()) {
fprintf(stdout, is_value_hex_ ? "0x%s\n" : "%s\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this printed line include the key as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just did this to be consistent with Get() output, but I do think having key<>value combo here would be nicer since we have multiple keys as input

@facebook-github-bot
Copy link
Contributor

@jaykorean merged this pull request in 59f4cbe.

@jaykorean jaykorean deleted the multiget_in_ldb branch January 29, 2024 21:51
@jaykorean jaykorean mentioned this pull request Feb 5, 2024
Closed
facebook-github-bot pushed a commit that referenced this pull request Feb 6, 2024
@jaykorean @facebook-github-bot
Multiget LDB Followup (#12332)
0088f77 
Summary:
# 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

Pull Request resolved: #12332

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
```

Reviewed By: jowlyzhang

Differential Revision: D53450164

Pulled By: jaykorean

fbshipit-source-id: 9ccec78ad3695e65b1ed0c147c7cbac502a1bd48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jowlyzhang jowlyzhang jowlyzhang left review comments

@hx235 hx235 hx235 approved these changes

@ajkr ajkr Awaiting requested review from ajkr

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 @jowlyzhang @hx235