Fix batch_put ttl issue by limbooverlambda · Pull Request #457 · tikv/client-rust

limbooverlambda · 2024-06-17T23:02:20Z

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

Signed-off-by: limbooverlambda <schakra1@gmail.com>

pingyu · 2024-06-22T08:01:52Z

+        // 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<_, _>>();


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.

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.

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

Signed-off-by: limbooverlambda <schakra1@gmail.com>

pingyu

Rest LGTM.

Signed-off-by: limbooverlambda <schakra1@gmail.com>

limbooverlambda · 2024-06-30T18:47:05Z

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

pingyu

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

pingyu · 2024-07-01T00:47:03Z


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


Suggested change

pingyu · 2024-07-01T00:47:27Z

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


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));

Signed-off-by: limbooverlambda <schakra1@gmail.com>

limbooverlambda · 2024-07-01T01:28:05Z

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.

pingyu

LGTM~
Thanks for you contribution !

* fixing the shard issue with batch_put Signed-off-by: limbooverlambda <schakra1@gmail.com> * PR feedback Signed-off-by: limbooverlambda <schakra1@gmail.com> * more make check fixes Signed-off-by: limbooverlambda <schakra1@gmail.com> * removing redundant map Signed-off-by: limbooverlambda <schakra1@gmail.com> * more PR feedback Signed-off-by: limbooverlambda <schakra1@gmail.com> * slight formatting change and remove another redundant clone Signed-off-by: limbooverlambda <schakra1@gmail.com> --------- Signed-off-by: limbooverlambda <schakra1@gmail.com> Co-authored-by: Ping Yu <yuping@pingcap.com> Signed-off-by: Ping Yu <yuping@pingcap.com>

limbooverlambda force-pushed the limbooverlambda/fix-batch-put-ttl-issue branch from 86e80bc to d751fcc Compare June 17, 2024 23:14

limbooverlambda mentioned this pull request Jun 17, 2024

BatchPut panics when keys are spread across regions #458

Closed

limbooverlambda force-pushed the limbooverlambda/fix-batch-put-ttl-issue branch from 00f6ff1 to 2d24519 Compare June 17, 2024 23:24

fixing the shard issue with batch_put

767e930

Signed-off-by: limbooverlambda <schakra1@gmail.com>

limbooverlambda force-pushed the limbooverlambda/fix-batch-put-ttl-issue branch from 2d24519 to 767e930 Compare June 17, 2024 23:29

pingyu reviewed Jun 22, 2024

View reviewed changes

limbooverlambda and others added 4 commits June 26, 2024 00:26

PR feedback

d6bd8a4

Signed-off-by: limbooverlambda <schakra1@gmail.com>

more make check fixes

463024c

Signed-off-by: limbooverlambda <schakra1@gmail.com>

removing redundant map

390e7c7

Signed-off-by: limbooverlambda <schakra1@gmail.com>

Merge branch 'master' into limbooverlambda/fix-batch-put-ttl-issue

9c907c9

pingyu reviewed Jun 29, 2024

View reviewed changes

more PR feedback

9d76994

Signed-off-by: limbooverlambda <schakra1@gmail.com>

pingyu reviewed Jul 1, 2024

View reviewed changes

slight formatting change and remove another redundant clone

74133f9

Signed-off-by: limbooverlambda <schakra1@gmail.com>

pingyu approved these changes Jul 1, 2024

View reviewed changes

pingyu merged commit 53e7a29 into tikv:master Jul 1, 2024


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

	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));

Conversation

limbooverlambda commented Jun 17, 2024

Uh oh!

pingyu Jun 22, 2024

Choose a reason for hiding this comment

Uh oh!

limbooverlambda Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

limbooverlambda Jun 28, 2024

Choose a reason for hiding this comment

Uh oh!

pingyu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

limbooverlambda commented Jun 30, 2024

Uh oh!

pingyu left a comment

Choose a reason for hiding this comment

Uh oh!

pingyu Jul 1, 2024

Choose a reason for hiding this comment

Uh oh!

limbooverlambda Jul 1, 2024

Choose a reason for hiding this comment

Uh oh!

pingyu Jul 1, 2024

Choose a reason for hiding this comment

Uh oh!

limbooverlambda Jul 1, 2024

Choose a reason for hiding this comment

Uh oh!

limbooverlambda commented Jul 1, 2024

Uh oh!

pingyu left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants