add a crate k2v client #303

Merged
lx merged 5 commits from trinity-1686a/garage:k2v-client into main 2022-05-18 20:24:15 +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
trinity-1686a added 1 commit 2022-05-14 11:16:38 +00:00
continuous-integration/drone/pr Build is failing Details
dd55af7efc
initial implementation of a k2v client
trinity-1686a reviewed 2022-05-14 11:19:22 +00:00
@ -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>(
Author
Owner

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
Owner

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

GJ, just a small remark below. I'll fix the documentation for ReadIndex.
@ -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");
Owner

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.
Author
Owner

I noticed when testing, but apparently I forgot to commit

I noticed when testing, but apparently I forgot to commit
lx marked this conversation as resolved
@ -0,0 +197,4 @@
Ok(())
}
// TODO poke team, draft doc outdated fot the return type of this endpoint
Owner

thx

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
@ -11,2 +11,3 @@
"src/garage"
"src/garage",
"src/k2v-client",
]
Owner

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
@ -0,0 +4,4 @@
use clap::{Parser, Subcommand};
/// Simple program to greet a person
Owner

I think this comment is wrong

I think this comment is wrong
Author
Owner

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
@ -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",
Owner

found*

found*
trinity-1686a marked this conversation as resolved
@ -0,0 +255,4 @@
/// Return only keys where conflict happened
#[clap(short, long)]
conflicts_only: bool,
/// Return only keys storing tombstones
Owner

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

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
@ -0,0 +350,4 @@
});
let stdout = std::io::stdout();
serde_json::to_writer(stdout, &json).unwrap();
Owner

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
@ -0,0 +360,4 @@
println!(
"{}: {},{},{},{}",
k, v.entries, v.conflicts, v.values, v.bytes
);
Owner

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?
Author
Owner

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
@ -0,0 +438,4 @@
}
} else {
false
};
Owner

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
trinity-1686a marked this conversation as resolved
@ -0,0 +503,4 @@
#[serde(default)]
pub conflicts_only: bool,
#[serde(default)]
pub include_tombstones: bool,
Owner

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
continuous-integration/drone/pr Build is passing Details
42ef5cde30
address review comments
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
trinity-1686a added this to the (deleted) milestone 2022-05-23 13:19:04 +00:00
Sign in to join this conversation.
No description provided.