add a crate k2v client #303

trinity-1686a · 2022-05-14T11:16:22Z

trinity-1686a commented

2022-05-14 11:16:22 +00:00

The code for a working client is there, in the future having a CLI along the way would be nice too.

lib.rs could use getting split in modules, but I'm not sure how exactly

The code for a working client is there, in the future having a CLI along the way would be nice too. lib.rs could use getting split in modules, but I'm not sure how exactly

👍 1 🚀 1

trinity-1686a added 1 commit 2022-05-14 11:16:38 +00:00

initial implementation of a k2v client

continuous-integration/drone/pr Build is failing

Details

dd55af7efc

trinity-1686a reviewed 2022-05-14 11:19:22 +00:00

src/k2v-client/src/lib.rs Outdated

					
				@ -0,0 +200,4 @@

					// TODO poke team, draft doc outdated fot the return type of this endpoint

					/// Perform a ReadIndex request, listing partition key which have at least one associated

					/// sort key, and which matches the filter.

					pub async fn read_index<'a>(

trinity-1686a commented

2022-05-14 11:19:21 +00:00

The documentation says the response is

{
  prefix: null,
  start: null,
  end: null,
  limit: null,
  reverse: false,
  partitionKeys: [
    { pk: "keys", n: 3043 },
    { pk: "mailbox:INBOX", n: 42 },
    { pk: "mailbox:Junk", n: 2991 },
    { pk: "mailbox:Trash", n: 10 },
    { pk: "mailboxes", n: 3 },
  ],
  more: false,
  nextStart: null,
}

but actually partitionKeys values are {pk: "keys", entries: 1, conflicts: 1, values: 1, bytes: 1}

The documentation says the response is ```json { prefix: null, start: null, end: null, limit: null, reverse: false, partitionKeys: [ { pk: "keys", n: 3043 }, { pk: "mailbox:INBOX", n: 42 }, { pk: "mailbox:Junk", n: 2991 }, { pk: "mailbox:Trash", n: 10 }, { pk: "mailboxes", n: 3 }, ], more: false, nextStart: null, } ``` but actually partitionKeys values are `{pk: "keys", entries: 1, conflicts: 1, values: 1, bytes: 1}`

lx marked this conversation as resolved

trinity-1686a force-pushed k2v-client from 2e614a2ad9 to 149695e144

2022-05-14 13:47:30 +00:00

Compare

trinity-1686a force-pushed k2v-client from 149695e144 to d0e3434104

2022-05-14 13:56:50 +00:00

Compare

lx approved these changes 2022-05-16 09:46:24 +00:00

lx left a comment

GJ, just a small remark below. I'll fix the documentation for ReadIndex.

src/k2v-client/src/lib.rs Outdated

					
				@ -0,0 +117,4 @@

						req.add_param("sort_key", sort_key);

						req.add_param("causality_token", &causality.0);

						req.add_param("timeout", &timeout.as_secs().to_string());

						req.add_header(ACCEPT, "application/octet-stream, application/json;q=0.9");

lx commented

2022-05-16 09:08:22 +00:00

The server doesn't handle q=0.9, this probably breaks k2v in its current state. In all cases when both are specified, application/octet-stream is already preferred when possible.

The server doesn't handle `q=0.9`, this probably breaks k2v in its current state. In all cases when both are specified, `application/octet-stream` is already preferred when possible.

trinity-1686a commented

2022-05-16 15:10:36 +00:00

I noticed when testing, but apparently I forgot to commit

lx marked this conversation as resolved

src/k2v-client/src/lib.rs

					
				@ -0,0 +197,4 @@

						Ok(())

					}

					// TODO poke team, draft doc outdated fot the return type of this endpoint

lx commented

2022-05-16 09:11:59 +00:00

thx

lx marked this conversation as resolved

trinity-1686a force-pushed k2v-client from d15f3ff083 to eca192af43

2022-05-16 16:26:36 +00:00

Compare

trinity-1686a requested review from lx 2022-05-16 17:24:31 +00:00

lx requested changes 2022-05-17 09:25:49 +00:00

Cargo.toml

					
				@ -11,2 +11,3 @@

					"src/garage"

					"src/garage",

					"src/k2v-client",

				]

lx commented

2022-05-17 08:29:03 +00:00

In the current state this breaks cargo run in the root project directory:

error: `cargo run` could not determine which binary to run. Use the `--bin` option to specify a binary, or the `default-run` manifest key.
available binaries: garage, k2v-cli

To fix this, just add:

default-members = ["src/garage"]

In the current state this breaks `cargo run` in the root project directory: ``` error: `cargo run` could not determine which binary to run. Use the `--bin` option to specify a binary, or the `default-run` manifest key. available binaries: garage, k2v-cli ``` To fix this, just add: ```toml default-members = ["src/garage"] ```

trinity-1686a marked this conversation as resolved

src/k2v-client/src/bin/k2v-cli.rs

					
				@ -0,0 +4,4 @@

				use clap::{Parser, Subcommand};

				/// Simple program to greet a person

lx commented

2022-05-17 08:55:42 +00:00

I think this comment is wrong

trinity-1686a commented

2022-05-17 12:25:10 +00:00

oops that's a copy-past from the doc

oops that's a copy-past from [the doc](https://docs.rs/clap/latest/clap/#example)

trinity-1686a marked this conversation as resolved

src/k2v-client/src/bin/k2v-cli.rs

					
				@ -0,0 +167,4 @@

							let mut val = val.value;

							if val.len() != 1 {

								eprintln!(

									"Raw mode can only read non-concurent values, fond {} values, expected 1",

lx commented

2022-05-17 09:16:18 +00:00

found*

trinity-1686a marked this conversation as resolved

src/k2v-client/src/bin/k2v-cli.rs

					
				@ -0,0 +255,4 @@

					/// Return only keys where conflict happened

					#[clap(short, long)]

					conflicts_only: bool,

					/// Return only keys storing tombstones

lx commented

2022-05-17 09:02:48 +00:00

The comment for this should be: "also include keys storing only tombstones", it doesn't list only these keys, just it includes them as well

trinity-1686a marked this conversation as resolved

src/k2v-client/src/bin/k2v-cli.rs

					
				@ -0,0 +350,4 @@

								});

								let stdout = std::io::stdout();

								serde_json::to_writer(stdout, &json).unwrap();

lx commented

2022-05-17 09:18:57 +00:00

I think it would be nice to use to_writer_pretty here and everywhere where JSON is produced

I think it would be nice to use `to_writer_pretty` here and everywhere where JSON is produced

trinity-1686a marked this conversation as resolved

src/k2v-client/src/bin/k2v-cli.rs Outdated

					
				@ -0,0 +360,4 @@

									println!(

										"{}: {},{},{},{}",

										k, v.entries, v.conflicts, v.values, v.bytes

									);

lx commented

2022-05-17 09:20:37 +00:00

We have a simple table formatting function in garage/cli/util.rs, maybe reuse that?

We have a simple table formatting function in `garage/cli/util.rs`, maybe reuse that?

trinity-1686a commented

2022-05-17 12:29:54 +00:00

I think format_table() should be moved to garage_util for that. Having k2v-client depends on all of the server code sounds bad

I think `format_table()` should be moved to garage_util for that. Having k2v-client depends on all of the server code sounds bad

trinity-1686a marked this conversation as resolved

src/k2v-client/src/bin/k2v-cli.rs

					
				@ -0,0 +438,4 @@

								}

							} else {

								false

							};

lx commented

2022-05-17 09:23:35 +00:00

I think this can lead to some confusion, because limit = 1 and singleItem = true do not mean the same thing.

If my items are B, C and D:

start = A, limit = 1 will return B (it looks for the first item, starting from A)
start = A, singleItem = true will return nothing (it looks for A exactly)

Here deleting a single item is already handled by the delete subcommand, so I think we should just forbid the usage of limit in DeleteRange

I think this can lead to some confusion, because `limit = 1` and `singleItem = true` do not mean the same thing. If my items are B, C and D: - `start = A, limit = 1` will return B (it looks for the first item, starting from A) - `start = A, singleItem = true` will return nothing (it looks for A exactly) Here deleting a single item is already handled by the `delete` subcommand, so I think we should just forbid the usage of `limit` in DeleteRange

👍 1

trinity-1686a marked this conversation as resolved

src/k2v-client/src/lib.rs Outdated

					
				@ -0,0 +503,4 @@

					#[serde(default)]

					pub conflicts_only: bool,

					#[serde(default)]

					pub include_tombstones: bool,

lx commented

2022-05-17 09:05:58 +00:00

This field isn't called include_tombstones but just tombstones (currently the -t flag is not working in k2v-cli)

This field isn't called `include_tombstones` but just `tombstones` (currently the `-t` flag is not working in k2v-cli)

trinity-1686a added 1 commit 2022-05-18 16:37:20 +00:00

address review comments

continuous-integration/drone/pr Build is passing

Details

42ef5cde30

trinity-1686a requested review from lx 2022-05-18 18:50:56 +00:00

lx merged commit 64c193e3db into main

2022-05-18 20:24:15 +00:00

lx referenced this pull request from a commit

2022-05-18 20:24:15 +00:00

Add a K2V client library and CLI (#303)

trinity-1686a added this to the (deleted) milestone 2022-05-23 13:19:04 +00:00

Sign in to join this conversation.

No reviewers

No milestone

No project

No assignees

2 participants

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Deuxfleurs/garage#303