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

Fix batch_put ttl issue #457

Merged

Conversation

limbooverlambda
Copy link
Contributor

There is a bug in the client where the ttls are not properly sharded for RawBatchPuts. This essentially means that if the batch is spread across multiple regions, the ttls for the batch specific to the shard is not properly aligned. This is an issue when the request ends up in the TiKV node specific to the region and fails because of the misalignment of the Vec<TTLs> with its corresponding Vec<KvPair>. The fix was to change the sharding logic for RawBatchPutRequest so that the TTLs are aligned with the KvPair. Maintaining a mapping of the kvpair to its corresponding ttl in the Shard output.

  • Added unit tests for the batch_put
  • Ran integ tests to confirm:
warning: `tikv-client` (lib) generated 4 warnings (run `cargo fix --lib -p tikv-client` to apply 2 suggestions)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.61s
     Running tests/integration_tests.rs (target/debug/deps/integration_tests-ae58f87a6b1272e6)

running 26 tests
test raw_bank_transfer ... ok
test raw_req ... ok
test txn_crud ... ok
test txn_batch_mutate_optimistic ... ok
test raw_ttl ... ok
test raw_cas ... ok
test txn_bank_transfer ... ok
test txn_batch_mutate_pessimistic ... ok
test txn_get_for_update ... ok
test raw_write_million ... 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 60.74s


@limbooverlambda limbooverlambda force-pushed the limbooverlambda/fix-batch-put-ttl-issue branch from 2d24519 to 767e930 Compare June 17, 2024 23:29
// Maintain a map of the pair and its associated ttl
let kvs = self.pairs.clone();
let kv_pair = kvs.into_iter().map(Into::<KvPair>::into);
let kv_ttl = kv_pair.zip(self.ttls.clone()).collect::<HashMap<_, _>>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

store_stream_for_keys is able to handle data of keys along with values and ttls. Mapping by Hash map is not necessary, and it's not efficient as well.

Please refer to PrewriteRequest::shards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review. Didn't realize store_stream_for_keys has this capability. Removed the HashMap to collect the ttls for the kv_pair.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @pingyu, I have made the changes. Please let me know if there's anything else I need to consider for this PR.

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.

Rest LGTM.

src/kv/key.rs Outdated Show resolved Hide resolved
src/raw/requests.rs Outdated Show resolved Hide resolved
src/raw/requests.rs Outdated Show resolved Hide resolved
src/raw/requests.rs Outdated Show resolved Hide resolved
src/raw/requests.rs Outdated Show resolved Hide resolved
src/raw/requests.rs Outdated Show resolved Hide resolved
src/store/mod.rs Outdated Show resolved Hide resolved
Signed-off-by: limbooverlambda <[email protected]>
@limbooverlambda
Copy link
Contributor Author

Thanks for helping me clean up this PR @pingyu. Addressed your comments.

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.

I'm sorry I missed another suggestion in last comments. The clone in comparation should also be un-necessary.

@@ -7,10 +7,12 @@ use std::time::Duration;

use async_trait::async_trait;
use futures::stream::BoxStream;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

.zip(ttls)
.map(|(kv, ttl)| KvPairTTL(kv, ttl))
.collect();
kv_ttl.sort_by(|a, b| a.0.key.clone().cmp(&b.0.key));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
kv_ttl.sort_by(|a, b| a.0.key.clone().cmp(&b.0.key));
kv_ttl.sort_by(|a, b| a.0.key.cmp(&b.0.key));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@limbooverlambda
Copy link
Contributor Author

I'm sorry I missed another suggestion in last comments. The clone in comparation should also be un-necessary.

Yeah, I should have caught that. Removed.

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~
Thanks for you contribution !

@pingyu pingyu merged commit 53e7a29 into tikv:master Jul 1, 2024
4 checks passed
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.

2 participants