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

Adding a reverse scan API for raw client #441

Merged
merged 3 commits into from
Feb 21, 2024
Merged

Conversation

rahulrane50
Copy link
Contributor

@rahulrane50 rahulrane50 commented Feb 9, 2024

Issue : #440

Problem statement :
Adding a support for reverse scan public API in raw rust client.

Solution :

  1. Added a new public API in raw client.
  2. Add flag all the way through until new raw scan request is created. The tikv service scan API already has a field for reverse flag. Set it properly based on what caller is passing.

Tests :

cargo build
cargo clippy && cargo fmt
cargo test


PD_ADDRS="127.0.0.1:2379" cargo test --package tikv-client --test integration_tests --features integration-tests
   Compiling tikv-client v0.3.0 (/Users/rrane/work/spiral-repos/client-rust)
    Finished test [unoptimized + debuginfo] target(s) in 10.41s
     Running tests/integration_tests.rs (target/debug/deps/integration_tests-e4cb8a4e124a5c59)

running 26 tests
test raw_bank_transfer ... ok
test raw_cas ... ok
test raw_req ... ok
test raw_ttl ... ok
test raw_write_million ... ok
test txn_bank_transfer ... ok
test txn_batch_mutate_optimistic ... ok
test txn_batch_mutate_pessimistic ... ok
test txn_crud ... ok
test txn_get_for_update ... ok
test txn_get_timestamp ... ok
test txn_insert_duplicate_keys ... ok
test txn_key_exists ... ok
test txn_lock_keys ... ok
test txn_lock_keys_error_handle ... ok
test txn_pessimistic ... ok
test txn_pessimistic_delete ... ok
test txn_pessimistic_heartbeat ... ok
test txn_pessimistic_rollback ... ok
test txn_read ... ok
test txn_scan ... ok
test txn_scan_reverse ... ok
test txn_scan_reverse_multi_regions ... ok
test txn_split_batch ... ok
test txn_unsafe_destroy_range ... ok
test txn_update_safepoint ... ok

test result: ok. 26 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 59.34s

@rahulrane50
Copy link
Contributor Author

rahulrane50 commented Feb 9, 2024

CI integration test failure - Not sure if this is flaky test but it passes locally.

PD_ADDRS="127.0.0.1:2379" cargo test --package tikv-client --test integration_tests --features integration-tests

     Running tests/integration_tests.rs (target/debug/deps/integration_tests-e4cb8a4e124a5c59)

running 26 tests
test raw_bank_transfer ... ok
test txn_batch_mutate_pessimistic ... ok
test txn_crud ... ok
test txn_bank_transfer ... ok
test raw_req ... ok
test txn_batch_mutate_optimistic ... ok
test txn_get_for_update ... ok
test raw_write_million ... ok
test raw_cas ... ok
test raw_ttl ... ok
test txn_get_timestamp ... ok
test txn_insert_duplicate_keys ... ok
test txn_key_exists ... ok
test txn_lock_keys ... ok
test txn_lock_keys_error_handle ... ok
test txn_pessimistic ... ok
test txn_pessimistic_delete ... ok
test txn_pessimistic_heartbeat ... ok
test txn_pessimistic_rollback ... ok
test txn_read ... ok
test txn_scan ... ok
test txn_scan_reverse ... ok
test txn_scan_reverse_multi_regions ... ok
test txn_split_batch ... ok
test txn_unsafe_destroy_range ... ok
test txn_update_safepoint ... ok

test result: ok. 26 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 59.44s

@rahulrane50 rahulrane50 marked this pull request as ready for review February 9, 2024 01:00
@rahulrane50
Copy link
Contributor Author

cc @ekexium

src/raw/client.rs Outdated Show resolved Hide resolved
src/raw/client.rs Outdated Show resolved Hide resolved
@rahulrane50
Copy link
Contributor Author

@ekexium can you take another look please?

@pingyu
Copy link
Collaborator

pingyu commented Feb 20, 2024

  • Suggest to also add a scan_keys_reverse method for convenience.
  • Suggest to add integration test for the scene of reverse scan across regions. Please refer to Fix reverse scan for scene of multiple regions #438.
  • Could you kindly remove the comment of "TODO" here as well ? Thanks.

Rest LGTM~

Signed-off-by: Rahul Rane <[email protected]>
Copy link
Collaborator

@pingyu pingyu left a comment

Choose a reason for hiding this comment

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

LGTM~

@rahulrane50
Copy link
Contributor Author

Thanks for the review and approval @pingyu. can you please help me with merging?

@pingyu pingyu merged commit c6110dd into tikv:master Feb 21, 2024
4 checks passed
@pingyu
Copy link
Collaborator

pingyu commented Feb 21, 2024

Thanks for the review and approval @pingyu. can you please help me with merging?

Done. Thanks for your contribution ~

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.

3 participants