WIP Docs by Hoverbear · Pull Request #10 · tikv/client-rust

Hoverbear · 2018-11-19T21:07:56Z

(This is a WIP!)

Based off #7.

Adds some documentation, along with tested examples.

sunxiaoguang

Wow, you have done all the document part. It's fabulous.

sunxiaoguang · 2018-11-21T01:37:33Z

+/// * Write: Where MVCC and index related data are stored. Set by `[rocksdb.writecf]`.
+/// * Lock: Where lock information is stored. Set by `[rocksdb.lockcf]`.
+/// 
+/// Not providing a call a `ColumnFamily` means it will use the default value of `default`.


Is 'a call' typo?

sunxiaoguang · 2018-11-21T02:48:40Z

+    }
+}
+
+impl<'a> From<&'a [u8]> for KeyRef<'a> {


How about implement From for reference to sized array smaller than 32. Like what rust std library does for array.
This way people can write code like this

let from_bytes = KeyRef::from(&b"TiKV");

instead of this

let from_bytes = KeyRef::from(&b"TiKV"[..]);

sunxiaoguang · 2018-11-21T02:55:25Z

-    type Item = Vec<KvPair>;
+impl<'client, 'keys: 'client, Iter> Future for BatchGet<'client, 'keys, Iter> 
+where Iter: Iterator<Item=KeyRef<'keys>> {
+    type Item = ();


I guess this is a copy paste error. It is supposed to resolve to key/value pairs isn't it?

sunxiaoguang · 2018-11-21T04:20:51Z

 }

-impl<'a> Future for Scan<'a> {
+impl<'client, 'keys: 'client, Bounds> Future for Scan<'client, 'keys, Bounds>


Just realized I only made Scanner in transaction api a Stream. Is implementing Stream better than Future?

sunxiaoguang · 2018-11-21T05:28:58Z

+    /// ```
+    pub fn scan<'client, 'keys, Bounds>(&'client self, range: Bounds, limit: impl Into<Option<u32>>) -> Scan<'client, 'keys, Bounds> 
+    where Bounds: RangeBounds<KeyRef<'keys>> {
+        use std::u32::MAX;


If the limit could be potentially large, Scan and BatchScan should definitely be Stream so it can do multiple round of scan. Otherwise the default value of MAX will blow up memory.

Unfortunately, the scan rpc returns RepeatedField of KvPair. To make it a Stream, tikv/tikv#3727 needs to be solved first.

gregwebs · 2018-11-27T05:28:43Z

+    /// ```rust
+    /// # use tikv_client::Config;
+    /// let config = Config::new(vec!["192.168.0.100:2379", "192.168.0.101:2379"])
+    ///     .with_security("root.ca", "internal.cert", "internal.key");


Three arguments of the same type means it is easy to put them in the wrong order. I would definitely keep it as one builder though. I would either newtype some of the arguments or take a struct with meaningfully named fields (essentially Rust does not have named arguments). Internally I would also use a struct so there is just one Option instead of three.

Having a dedicated type with constructor is equally tricky as this one. Therefore the type will have to make everything public to make it possible to construct by field name.
How about break it into three methods to set them separately.

Three separate methods doesn't capture the nature of this: there are three arguments, and they must be given as all or none. The concern with the constructor is making it public? Won't any solution need to make something public?

sticnarf · 2018-11-30T17:49:28Z

 pub struct BatchScan<'a> {
    client: &'a Client,
-    ranges: Vec<(Key, Key)>,
+    ranges: Vec<(Bound<Key>, Bound<Key>)>,


What's your concern about collecting the iterator into a Vec? Is there any drawback of storing an iterator here (like the previous version)?

I'd like to revisit doing that :)

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

sticnarf · 2018-12-14T09:27:18Z

+/// The only way to avoid this would be to only accept two `Bound<T>` arguments, not one
+/// `RangeBound<T>` argument. This would mean `"abc"..="xyz"` would not be possible.
+/// 
+// TODO: If you feel tricky please try to remove this clone!


I introduce another trait to avoid this clone. Would you please see if this looks good to you?

https://github.com/sticnarf/tikv-client-rust/tree/key-range-trait

PTAL @sunxiaoguang

Yea, I think this way is better it can avoid extra clone when bound is already a Key however make it possible to use non Key type which can be converted to Key.

Great idea, I'll integrate this @sticnarf

siddontang · 2018-12-17T15:01:34Z

+use quick_error::quick_error;
+use std::{error, result};

 quick_error!{


IMO, I want to try Failure instead of quick_error :-)

Let me take a look into failure.

I also do! :)

Hoverbear · 2018-12-31T19:21:09Z

Closed in favor of #13

Hoverbear force-pushed the docs branch from e41af51 to 3c28274 Compare November 21, 2018 01:15

sunxiaoguang reviewed Nov 21, 2018

View reviewed changes

This was referenced Nov 22, 2018

Question: Converting &[u8] to Key #11

Closed

TiKV Rust Client RFC tikv/rfcs#7

Open

gregwebs reviewed Nov 27, 2018

View reviewed changes

Hoverbear force-pushed the docs branch from 3c28274 to 497b6e3 Compare November 30, 2018 01:03

sticnarf reviewed Nov 30, 2018

View reviewed changes

Hoverbear added 6 commits November 30, 2018 12:05

Bump to 2018 edition.

6bb3731

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

Add toolchain

81e5d1a

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

Fix examples

eb48139

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

fmt and lint

4630ebc

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

Documentation, first pass

176c887

Add more documentation for raw client

fd2f08b

Hoverbear force-pushed the docs branch from 497b6e3 to 7f5a33c Compare December 13, 2018 19:16

sticnarf reviewed Dec 14, 2018

View reviewed changes

siddontang reviewed Dec 17, 2018

View reviewed changes

Document various parts of the raw/transaction interface.

9034c02

Hoverbear force-pushed the docs branch from 7f5a33c to 547db25 Compare December 27, 2018 22:58

Hoverbear added 2 commits December 27, 2018 15:45

More docs, sticnarf feedback.

faeffb3

Document transaction module

2bbe7e1

Hoverbear force-pushed the docs branch from 547db25 to 2bbe7e1 Compare December 28, 2018 01:05

Hoverbear closed this Dec 31, 2018

Hoverbear deleted the docs branch December 31, 2018 19:21

sticnarf mentioned this pull request Jan 16, 2019

Use failure for error management #23

Merged

Conversation

Hoverbear commented Nov 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sunxiaoguang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hoverbear commented Dec 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Hoverbear commented Nov 19, 2018 •

edited

Loading