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

TiKV Command Line RFC #21

Open
wants to merge 12 commits into
base: master
from

Conversation

@Hoverbear
Copy link
Member

commented Feb 26, 2019

Based on #7 we discussed having a command line client.

This RFC proposes one! How exciting.

@Hoverbear Hoverbear self-assigned this Feb 26, 2019

@Hoverbear Hoverbear requested a review from nrc Feb 26, 2019

@Hoverbear

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

@sunxiaoguang can you take a look since it's based off your RFC? :)

TiKV Command Line RFC
Signed-off-by: Ana Hobden <operator@hoverbear.org>

@Hoverbear Hoverbear force-pushed the tikv-client-cli branch from 485de24 to 4bdc822 Feb 26, 2019

@sunxiaoguang

This comment has been minimized.

Copy link

commented Feb 26, 2019

@sunxiaoguang can you take a look since it's based off your RFC? :)

Sure. I will have a look in a moment.

Fixup log levels and add API note.
Signed-off-by: Ana Hobden <operator@hoverbear.org>
@Hoverbear

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2019

After some discussion with @nrc we think that it is appropriate to default to transaction in most cases. So perhaps we should default to transaction and allow the user to configure it via the config with --mode

Hoverbear added some commits Mar 5, 2019

Refinements to API
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Fix line length
Signed-off-by: Ana Hobden <operator@hoverbear.org>

@Hoverbear Hoverbear force-pushed the tikv-client-cli branch from 78f45ff to 68ee7e0 Mar 5, 2019

@Hoverbear

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2019

@brson

This comment has been minimized.

Copy link

commented Mar 20, 2019

Sweet idea, and fun.

A repl requires building a language and interpreter and there will probably be ongoing feature requests to expand its capabilities. It might be easier to create python bindings and reuse python's repl. Or maybe some other scripting language that's easy on the eyes.

@Hoverbear

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2019

😵 Then it's annoying to install and build. It's much easier to distribute a single musl binary or allow a simple cargo install instead of having to mess around with system packages and pip. Besides! Python bindings are a whole other project.

I suspect it won't be terrifically hard to create a little repl with clap's arg_matches and a loop. Might try to whip up a demo...

Hoverbear added some commits Mar 21, 2019

Structure one-off-txn mode better
Signed-off-by: Ana Hobden <operator@hoverbear.org>
@Hoverbear

This comment has been minimized.

Copy link
Member Author

commented Mar 21, 2019

@brson Here's a simple REPL example for interactive mode. It'll parse first token (blah, "blah blah", 123) as the KEY, optionallly the second as the VALUE using the normal clap parser and a simple lexer and readline interface:

use clap::{App, Arg};
use std::env::args_os;
use linefeed::{Interface, ReadResult};
use scanlex::{Scanner, Token};

fn main() {
    let repl_interface = Interface::new("tikv").unwrap();
    repl_interface.set_prompt("tikv > ").unwrap();

    let mut app = App::new("get")
        .arg(Arg::with_name("KEY")
            .required(true))
        .arg(Arg::with_name("VALUE")
            .required(false));

    let matches = app.get_matches_from_safe_borrow(args_os());
    println!("{:?}", matches);

    loop {
        if let Some(res) = repl_interface.read_line_step(None).unwrap() {
            match res {
                ReadResult::Input(input) => {
                    let scanner = Scanner::new(&input);

                    let tokens = scanner.map(|v| match v {
                        Token::Iden(x) | Token::Str(x) => x,
                        Token::Int(y) => format!("{}", y),
                        _ => panic!("No arg"),
                    });

                    let preface = vec![String::from("tikv")];
                    let iter = preface.into_iter().chain(tokens.into_iter());

                    let matches = app.get_matches_from_safe_borrow(iter);
                    println!("{:?}", matches);
                }
                ReadResult::Eof => {
                    break;
                },
                _ => {}
            }
        }
    }
    println!("{:?}", matches);
}

You can have a try and see that it feels fine to use and doesn't require a lot of work.

@brson

This comment has been minimized.

Copy link

commented Mar 23, 2019

Then it's annoying to install and build. It's much easier to distribute a single musl binary or allow a simple cargo install instead of having to mess around with system packages and pip. Besides! Python bindings are a whole other project.

That's a problem with Python specifically, yes. There may be other suitable languages.

@ice1000

This comment has been minimized.

Copy link
Member

commented May 2, 2019

For REPL I'll recommend rustyline which supports completion/hints/history.

@Hoverbear

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

Hi @sciamp-dev, thanks for volunteering to get involved with this project! We're really excited to learn with you and I'm looking forward to working with you directly.

I know you've already had a look at this RFC, but could you please verify it and give your feedback/approval?

@zhangjinpeng1987 @brson do you think you could take a look at this project and give feedback so our friend can get started? @siddontang do you think you could help me open a client-cli repo for this and give me maintainer status? I'll work with @sciamp-dev to create an initial implementation in the coming weeks.

@sciamp-dev

This comment has been minimized.

Copy link

commented May 3, 2019

Hi everyone, I'm exited to work with you, too! And thanks @Hoverbear :)

I agree with the use of RustyLine for REPL mode, reading the docs it looks more complete and it has more functionalities then other crates like liner or linefeed.
As for the idea of making Python bindings, if there's a solution in Rust I think it's better and more user-friendly.

@ice1000

This comment has been minimized.

Copy link
Member

commented May 3, 2019

I'd say making use of some existing Rust-implemented scripting languages and add binding for them or create our own PL are both good ideas.

@brson

This comment has been minimized.

Copy link

commented May 6, 2019

I think somebody should go ahead and get started — I'd rather have code written than not. Whatever that tool looks like will inevitably evolve and undoubtedly end up different.

So that's what I think should happen, but below are some things to consider.

DSLs have a mixed track record. I'm not though going to seriously advocate binding a scripting language for the DSL. The examples in this RFC are very simple, and if that ends up being the extent of the tool's functionality then a DSL is fine. DSLs can turn into bad general purpose languages.

The duality between the one-off mode and the repl probably imposes yet-unknown limits on the grammar.
The one-off mode accepts the tokens of its grammar as individual command line arguments, which has limitations compared to the arbitrary string parsing possible in the repl. Other scripting languages tend to have a flag that just accepts a single string containing script text. With the simple grammar implied here, just statements, keywords, and strings, no variables, no expressions, the duality between cli arguments and tokens looks neat. But that design might not hold up.

A limited language for the one-off mode can be augmented with other tools like jq, but that isn't true for the repl. The repl described is not powerful.

@brson

brson approved these changes May 6, 2019

@ice1000

ice1000 approved these changes May 6, 2019

@Hoverbear

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

🎉 @sciamp-dev please review and give feedback/approve this PR, I've reached out to the benevolent rulers of the TiKV org to create the repo for us. :)

@sciamp-dev

This comment has been minimized.

Copy link

commented May 6, 2019

Sure @Hoverbear ! I approve this PR, we can go ahead and start.
I agree with what @brson said. I'm doing some quick tests with RustyLine, PyO3(/cpython) and wasm-bindgen for binding with Python and JS. But I guess we'll choose/adapt considering whatever needs we'll have.

@Hoverbear

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

We (@siddontang , @sciamp-dev ) discussed this on our community chat and decided to start with this being part of the client-rust repo and making it an opt-in binary until the project fully matures. This will help it act as a nice test bed for the Rust client, and give it some extra maintenance eyes.

I also corrected some mistakes around tikv-cli vs tikv-client.

I also added a note about output formats as we do have some commonly used formats and we need to consider our non-UTF-8 users. Please, take a look! :)

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

@Hoverbear Hoverbear force-pushed the tikv-client-cli branch from d5d33f2 to 43e4fe9 May 8, 2019

Hoverbear added some commits Jul 29, 2019

Reflect sciamp-dev feedback
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.