The initial version of Raw KV implementation by sunxiaoguang · Pull Request #14 · tikv/client-rust

sunxiaoguang · 2018-12-31T14:13:42Z

No description provided.

Hoverbear

With the understanding this is an initial version and some things will be refined as we move foward, this looks really good. :) A few small nits.

Hoverbear · 2019-01-01T00:05:42Z

Fixes #12

sunxiaoguang · 2019-01-01T08:52:14Z

The tests requires a TiKV cluster running and PD can be connected to with endpoint 127.0.0.1:2379. I'm trying to add a mock TiKV server to make integration test easier, but I guess that will take quite some time.
@Hoverbear @brson

brson

@sunxiaoguang this is a huge amount of high quality work. Thanks for being so careful with the design and documentation.

I've reviewed through the rpc client so far, and will continue another day.

I left a number of comments about things that might be changed (a lot of them are hidden behind an expando on this page). The most important I think is the question about wraparound in the conversion from bounds to ranges: https://github.com/tikv/client-rust/pull/14/files#r245455268

Awesome stuff.

brson · 2019-01-05T00:37:35Z

+    /// let req = connected_client.scan(inclusive_range, 2);
+    /// let result: Vec<KvPair> = req.wait().unwrap();
+    /// ```
+    pub fn scan(&self, range: impl KeyRange, limit: impl Into<Option<u32>>) -> Scan {


Is the impl Into argument really required here? The more clever generics in a type signature the harder to understand.

Is this a clever trick to allow passing either a u32 or None? If so, my instinct is to dislike it (sorry :)) - I lean toward being explicit over clever type-level ergonomics hacks. I'm curious if this pattern is well-represented in the modern Rust ecosystem.

This is the same interface in Golang client which always take a limit.

func (c *RawKVClient) Scan(startKey []byte, limit int) (keys [][]byte, values [][]byte, err error)

We can do the same thing here.

BTW: The max limit can be changed in Golang client as MaxRawKVScanLimit is a public variable. I'm not sure if we want to let user change this via Config or simply do not allow user to change it. I'm using a const right now and would like to hear comments from everybody about this.

I'm not even sure what this limit is, but I suspect I prefer giving users the option to not think about it, so either Option or Into. I don't have strong feelings about which one.

I think I don't know the domain well enough yet to have an opinion on exactly where it should be possible to set this value.

Since the underlying RPC protocol requires limit to be set, and there should be a rational upper bound. I suggest changing this argument to required. @Hoverbear @siddontang What do you think?

@sunxiaoguang I think that's fine. Let's make sure to expose a 'max'/easy const as tikv_client::NO_SCAN_LIMIT or something so users can find it easily.

brson · 2019-01-05T01:18:59Z

+
+    fn poll(&mut self) -> Poll<Self::Item, Self::Error> {
+        let config = &self.config;
+        let rpc = Arc::new(RpcClient::connect(config)?);


Is RpcClient::connect synchronous? From reading rpc::client it's not obviously asynchronous.

It is synchronous at this time. I thought it might be good to leave possibility to support asynchronous connect later on. That way people can implement proxy like applications that support connecting multiple TiKV cluster on the fly without blocking the core. Since it is probably something that's unlikely to happen in real life, if changing connect to blocking style is easier to understand and use, let's change it.

BTW: Thank you so much for all the suggestions, let me take a look and fix them.

It's fine for now, but istm that ideally everything running on an async event loop should be non-blocking. It's probably worth noting in the doc comments that this future is not asynchronous.

sunxiaoguang

@brson @Hoverbear I pushed another commit and try to fix the issues mentioned here. Please take a look, thanks.

brson · 2019-01-08T19:49:26Z

I may not get back to finishing my readthrough of this PR for a few days. For the sake of progress, since this is experimental code, I'm comfortable saying LGTM and moving forward. Otherwise I'll get back to it in a bit.

Signed-off-by: Xiaoguang Sun <sunxiaoguang@zhihu.com>

Signed-off-by: Ana Hobden <operator@hoverbear.org>

Hoverbear · 2019-01-11T20:03:47Z

Going to try to get some green CI builds.

Signed-off-by: Ana Hobden <operator@hoverbear.org>

Signed-off-by: Xiaoguang Sun <sunxiaoguang@zhihu.com>

Hoverbear

LGTM, still.

sunxiaoguang · 2019-01-13T02:09:10Z

LGTM, still.

@siddontang shell we merge it and leave the rest for future PRs?

siddontang · 2019-01-13T05:19:06Z

+    // This is *advanced usage* and should have some special considerations.
+    client
+        .delete(key.clone())
+        .cf(CUSTOM_CF)


IMO, I suggest removing cf here because now we can't let the user create CF dynamically. Providing cf here may confuse the users if they want to use other cf.

How about we change cf to private and remove it from example at this time?

@sunxiaoguang Maybe you can just remove it from the example for now? If people configure TiKV with custom CFs we must allow them to use them, but perhaps this part of the API can change later.

Hoverbear · 2019-01-13T22:03:03Z

Merging. CI is red due to the service brownout, but we're now seeing multiple PRs built off this one and that's not a good situation. @sunxiaoguang Can you please reflect @siddontang 's minor nit in another PR?

sunxiaoguang · 2019-01-14T00:41:48Z

Merging. CI is red due to the service brownout, but we're now seeing multiple PRs built off this one and that's not a good situation. @sunxiaoguang Can you please reflect @siddontang 's minor nit in another PR?

Sure, just did it. PTAL #21

My name is Sun Xiaoguang from Zhihu, where I help make a BigTable like system focusd on OLTP that is using TiKV. I've been contributing to TiKV and the library ecosystem for around a year now. I first touched the TiKV codebase since around March 2018. As part of my involvement with the project I've contributed several improvements to the TiKV Project: * [Added raw batch put/get/delete/scan](pingcap/kvproto#244) * [Changed `get_region` and `get_region_info` to use a common implementation](#3521) * [Print help message when starting tikv-ctl without argument](#3531) * [Clean up unused #[allow] directives](#3553) I also am the first and primary author of the [TiKV Rust Client](https://github.com/tikv/client-rust). You can see related PRs for that here: * [TiKV Rust Client RFC](tikv/rfcs#7) * [The initial version of Raw KV Implementation](tikv/client-rust#14) * [Change `raw::Client::get` to return `Option<Value>`](tikv/client-rust#24) * [Remove cf and batch_scan from `example/raw.rs`](tikv/client-rust#21) I also made the first PR to the mock TiKV project: [A basic implementation of mock TiKV API surface](tikv/mock-tikv#1) As part of my involvement, the TiKV team inquired if I was interested in becoming a maintainer for the project. This PR adds me to the list of maintainers. I have read and understand the expectations of maintainers described in the `GOVERNANCE.md` file. Signed-off-by: Xiaoguang Sun <sunxiaoguang@zhihu.com>

@sunxiaoguang

* Title: Add @sunxiaoguang to MAINTAINERS.md My name is Sun Xiaoguang from Zhihu, where I help make a BigTable like system focusd on OLTP that is using TiKV. I've been contributing to TiKV and the library ecosystem for around a year now. I first touched the TiKV codebase since around March 2018. As part of my involvement with the project I've contributed several improvements to the TiKV Project: * [Added raw batch put/get/delete/scan](pingcap/kvproto#244) * [Changed `get_region` and `get_region_info` to use a common implementation](#3521) * [Print help message when starting tikv-ctl without argument](#3531) * [Clean up unused #[allow] directives](#3553) I also am the first and primary author of the [TiKV Rust Client](https://github.com/tikv/client-rust). You can see related PRs for that here: * [TiKV Rust Client RFC](tikv/rfcs#7) * [The initial version of Raw KV Implementation](tikv/client-rust#14) * [Change `raw::Client::get` to return `Option<Value>`](tikv/client-rust#24) * [Remove cf and batch_scan from `example/raw.rs`](tikv/client-rust#21) I also made the first PR to the mock TiKV project: [A basic implementation of mock TiKV API surface](tikv/mock-tikv#1) As part of my involvement, the TiKV team inquired if I was interested in becoming a maintainer for the project. This PR adds me to the list of maintainers. I have read and understand the expectations of maintainers described in the `GOVERNANCE.md` file. Signed-off-by: Xiaoguang Sun <sunxiaoguang@zhihu.com>

@sunxiaoguang

* Title: Add @sunxiaoguang to MAINTAINERS.md My name is Sun Xiaoguang from Zhihu, where I help make a BigTable like system focusd on OLTP that is using TiKV. I've been contributing to TiKV and the library ecosystem for around a year now. I first touched the TiKV codebase since around March 2018. As part of my involvement with the project I've contributed several improvements to the TiKV Project: * [Added raw batch put/get/delete/scan](pingcap/kvproto#244) * [Changed `get_region` and `get_region_info` to use a common implementation](tikv#3521) * [Print help message when starting tikv-ctl without argument](tikv#3531) * [Clean up unused #[allow] directives](tikv#3553) I also am the first and primary author of the [TiKV Rust Client](https://github.com/tikv/client-rust). You can see related PRs for that here: * [TiKV Rust Client RFC](tikv/rfcs#7) * [The initial version of Raw KV Implementation](tikv/client-rust#14) * [Change `raw::Client::get` to return `Option<Value>`](tikv/client-rust#24) * [Remove cf and batch_scan from `example/raw.rs`](tikv/client-rust#21) I also made the first PR to the mock TiKV project: [A basic implementation of mock TiKV API surface](tikv/mock-tikv#1) As part of my involvement, the TiKV team inquired if I was interested in becoming a maintainer for the project. This PR adds me to the list of maintainers. I have read and understand the expectations of maintainers described in the `GOVERNANCE.md` file. Signed-off-by: Xiaoguang Sun <sunxiaoguang@zhihu.com>

sunxiaoguang changed the title ~~The initial version of Raw KV impleme~~ The initial version of Raw KV implementation Dec 31, 2018

sunxiaoguang force-pushed the sun/raw_kv_initial_version branch from e4b2df0 to 1fc98b0 Compare December 31, 2018 14:15

sunxiaoguang requested a review from Hoverbear December 31, 2018 14:18