Skip to content

Public API RFC#2

Closed
sunxiaoguang wants to merge 11 commits intosun/emptyfrom
public_api_rfc
Closed

Public API RFC#2
sunxiaoguang wants to merge 11 commits intosun/emptyfrom
public_api_rfc

Conversation

@sunxiaoguang
Copy link
Copy Markdown
Member

This is a pull request helping us to make comments on public API

Comment thread Cargo.toml
description = "The rust language implementation of TiKV client."

[lib]
name = "tikv_client"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

mostly, we name our Go library as ticlient, maybe we can use the same name here. :-)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

/cc @c4pt0r

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I had a little concern what if there are other storage system built on top of TiKV and we want to implement a Rust client for it as well?

Copy link
Copy Markdown
Member Author

@sunxiaoguang sunxiaoguang Oct 16, 2018

Choose a reason for hiding this comment

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

Although it's probably too early to say that. But I really pictured a day in my mind that we could build a distributed file system or maybe timeseries database on top of TiKV. And for the nature of it's protocol complexity, it's likely a dedicated client is preferred.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Can't believe that after more than a decade, HDFS probably wins in ecosystem but still loses in scalability due to HA NameNode architecture. Even Google's initial GFS has something better than HDFS. Although there are federation support but it sounds better to have a clustered NameNode natively which can be built on top of TiKV.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@siddontang I can only see TiClient used twice in TiDB's code: https://github.com/pingcap/tidb/search?q=TiClient&unscoped_q=TiClient.

Also in the name of one test file https://github.com/pingcap/tidb/blob/cb03f2bec151033a38dfd17715a42cf3728b0541/store/tikv/ticlient_test.go

I think using a different name than tikv_client risks the user not understanding what this is for. ticlient is ambiguous and could mean tidb or tispark or something else.

Comment thread src/raw.rs
use {Key, KeyRange, KvFuture, KvPair, Value};

pub trait Raw {
fn get<K, C>(&self, key: K, cf: C) -> KvFuture<Value>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe we can borrow the key here? This would possibly save the caller a vec.clone().

Copy link
Copy Markdown
Member Author

@sunxiaoguang sunxiaoguang Oct 16, 2018

Choose a reason for hiding this comment

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

The underlying grpc request is going to move this key anyway. And I found there is a suggestion in API Guideline saying if an argument is going to be moved, it's better to make it clear on function signature instead of making a clone/copy inside the implementation.

https://rust-lang-nursery.github.io/api-guidelines/flexibility.html#caller-decides-where-to-copy-and-place-data-c-caller-control

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

Signed-off-by: Xiaoguang Sun <sunxiaoguang@zhihu.com>
Signed-off-by: Xiaoguang Sun <sunxiaoguang@zhihu.com>
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