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

Use the rocksdb mget to improve the performance #1453

Merged
merged 2 commits into from
May 22, 2023

Conversation

zevin02
Copy link
Contributor

@zevin02 zevin02 commented May 21, 2023

fixes: #1441

  • Use RocksDB's MultiGet to improve mget performance of hash

fixes: apache#1441

- Use RocksDB's MultiGet to improve mget performance of hash

Signed-off-by: Zewen Xu <zevin9427@gmail.com>
mapleFU
mapleFU previously approved these changes May 21, 2023
Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

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

The code LGTM.

By the way, should we add a Storage::MultiGet without ColumnFamily like Get?

@zevin02
Copy link
Contributor Author

zevin02 commented May 21, 2023

I think I can add it

tisonkun
tisonkun previously approved these changes May 21, 2023
src/types/redis_hash.cc Outdated Show resolved Hide resolved
Signed-off-by: Zewen Xu <zevin9427@gmail.com>
@zevin02 zevin02 dismissed stale reviews from tisonkun and mapleFU via e17273e May 21, 2023 14:11
Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

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

Sorry, miss the NotFound part

@mapleFU
Copy link
Member

mapleFU commented May 21, 2023

@zevin02 Could you find or add a testcase for #1455 ?

@zevin02
Copy link
Contributor Author

zevin02 commented May 21, 2023

@zevin02 Could you find or add a testcase for #1455 ? @zevin02 你能为 #1455 找到或添加一个测试用例吗?

What does it mean? Do I need to add a test case for reproducing this issue? I think the PR I just modified already fixed this issue.

@git-hulk
Copy link
Member

Sorry, miss the NotFound part

😊 No problem. Great thanks for your review.

@infdahai
Copy link
Contributor

infdahai commented May 21, 2023

What does it mean? Do I need to add a test case for reproducing this issue? I think the PR I just modified already fixed this issue.

It means you can just write a test to record this process. (Add a test to the hash test)
https://github.com/apache/incubator-kvrocks/blob/fb0e3d42031a6e0691c7a736711d0a01d72bdb1a/tests/gocase/unit/type/hash/hash_test.go#L53

The test is just used to avoid this situation reproducing due to the code that follows.

@mapleFU
Copy link
Member

mapleFU commented May 21, 2023

--- FAIL: TestSlotMigrateSync (47.71s)
    --- FAIL: TestSlotMigrateSync/MIGRATE_-_Migrate_sync_with_(or_without)_all_kinds_of_timeouts (0.01s)
        slotmigrate_test.go:425: 
            	Error Trace:	/home/runner/work/incubator-kvrocks/incubator-kvrocks/tests/gocase/integration/slotmigrate/slotmigrate_test.go:425
            	Error:      	Not equal: 
            	            	expected: string("OK")
            	            	actual  : <nil>(<nil>)
            	Test:       	TestSlotMigrateSync/MIGRATE_-_Migrate_sync_with_(or_without)_all_kinds_of_timeouts
FAIL
exit status 1

Is the unittest failed related?

@zevin02
Copy link
Contributor Author

zevin02 commented May 21, 2023

--- FAIL: TestSlotMigrateSync (47.71s)
    --- FAIL: TestSlotMigrateSync/MIGRATE_-_Migrate_sync_with_(or_without)_all_kinds_of_timeouts (0.01s)
        slotmigrate_test.go:425: 
            	Error Trace:	/home/runner/work/incubator-kvrocks/incubator-kvrocks/tests/gocase/integration/slotmigrate/slotmigrate_test.go:425
            	Error:      	Not equal: 
            	            	expected: string("OK")
            	            	actual  : <nil>(<nil>)
            	Test:       	TestSlotMigrateSync/MIGRATE_-_Migrate_sync_with_(or_without)_all_kinds_of_timeouts
FAIL
exit status 1

Is the unittest failed related?

I did not make any changes to the relevant code and I don't know why there is an error.

@infdahai
Copy link
Contributor

It is related to #1452.

@zevin02
Copy link
Contributor Author

zevin02 commented May 21, 2023

emm,So what should I do

It is related to #1452.

@infdahai
Copy link
Contributor

Sometimes the test works and sometimes it doesn't. So just ignore it for now.

@mapleFU
Copy link
Member

mapleFU commented May 21, 2023

@zevin02 Patch LGTM. And the hash case is tested. Let's wait committer rerun the unittest tomorrow.

@zevin02
Copy link
Contributor Author

zevin02 commented May 21, 2023

What does it mean? Do I need to add a test case for reproducing this issue? I think the PR I just modified already fixed this issue.

It means you can just write a test to record this process. (Add a test to the hash test)

https://github.com/apache/incubator-kvrocks/blob/fb0e3d42031a6e0691c7a736711d0a01d72bdb1a/tests/gocase/unit/type/hash/hash_test.go#L53

The test is just used to avoid this situation reproducing due to the code that follows.

I can add this test case later

@git-hulk
Copy link
Member

Thanks all, the patch looks good to be merged.

@git-hulk git-hulk merged commit 60d13ef into apache:unstable May 22, 2023
@git-hulk git-hulk changed the title Feat: optimize MGet function for improved performance Use the rocksdb mget to improve the performance May 22, 2023
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.

Hash::MGet: use DB::MultiGet for hash fields
5 participants