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

Return the keyspace list with its associated evictor config #2

Closed
wants to merge 0 commits into from

Conversation

loginn
Copy link

@loginn loginn commented Nov 2, 2022

Works :>
Not sure about the need to assess errors that way as I dont expect these errors to happen at all but still worth having those checks I guess :>

We should probably be using another format to return that config as the parsing might be painful on the other end but I am not sure about what to use.
Another idea there is to have the config separated into a different Frame::String that gets appended like the name of the keyspace. However that makes it harder right now in the CLI to identify what is a keyspace and what is config.

Maybe have a config frame like this Frame::Config ?

Also it would probably be better to have some config struct with a Display impl but I'm not sure about that either so I guess this PR is for me to ask questions mostly :>

Copy link
Owner

@thetinygoat thetinygoat left a comment

Choose a reason for hiding this comment

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

Hi please go thorugh the added comments

src/command.rs Outdated
@@ -475,6 +475,7 @@ pub fn parse(frame: Frame) -> Result<Command, ParseCommandError> {
"ttl" => Ok(Command::Ttl(Ttl::parse(&mut parser)?)),
"ping" => Ok(Command::Ping),
"keyspaces" => Ok(Command::Keyspaces),
"export" => Ok(Command::Keyspaces),
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we need to add a new command, the keyspaces command should be used to construct the export. Basically have a flag on the frontend (CLI) to read the data sent from the keyspaces command and constuct the export. maybe something like this

keyspaces --export

src/db.rs Outdated
}

// Implementing Display is the easiest way to get a to_string() method
impl Display for Evictor {
Copy link
Owner

Choose a reason for hiding this comment

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

We don't need to impl Display, it will cause un-necessry allocations everytime. I suggest that we have global static variables like this.

static EVICTOR_NOP: &[u8] = b"NOP";
static EVICTOR_RANDOM: &[u8] = b"RANDOM";
static EVICTOR_LRU: &[u8] = b"LRU";

and create the string frame like this

Frame::String(Bytes::from_static(EVICTOR_RANDOM));

Copy link
Author

@loginn loginn Nov 3, 2022

Choose a reason for hiding this comment

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

We lose the benefit of making sure that we have a valid string for each enum variant though, it might be possible to have an enum variant without an implemented string and there might be some issue later.

How about implementing something like :

impl Evictor {
    fn to_str(&self) -> &'static str {
        match self {
            Evictor::Nop => "NOP",
            Evictor::Lru => "LRU",
            Evictor::Random => "RANDOM",
        }
    }
}

Copy link
Owner

Choose a reason for hiding this comment

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

Sounds, good. We can add a similar method to get a &[u8]. Also I think it's better to name the methods as as_str() and as_bytes().

src/db.rs Outdated
@@ -147,7 +164,11 @@ impl Db {
let handle = self.keyspaces.read();
let mut keyspaces = Vec::with_capacity(handle.keys().count());
for key in handle.keys() {
keyspaces.push(Frame::String(key.clone()))
Copy link
Owner

Choose a reason for hiding this comment

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

Our protocol supports maps, so we can send the response as list of maps, where each keyspace will have its config in a map.

Maps in our protocol are built using vectors where each even index is key and each odd index is the value and the vector is even sized.

The response can be constructed like this.

let mut keyspaces = Vec::new();
let mut map = Vec::new();
map.push(Frame::String(Bytes::from("name"))); // this is the key
map.push(Frame::String(Bytes::from("<keyspace_name>"))); // this is the value
map.push(Frame::String(Bytes::from("evictor"))); // this is the key
map.push(Frame::String(Bytes::from_static(EVICTOR_RANDOM))); // this is the value
keyspaces.push(Frame::Map(map));

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure that using a map as the response type makes sense as it is by nature unordered. We need to do the sorting on the utility side. Otherwise items won't come out in any guaranteed order, which leads to sometimes having weird sorts.

I think working with a Vec is probably the better way to do things 🤔

src/db.rs Outdated
keyspaces.push(Frame::String(key.clone()))
keyspaces.push(Frame::String(format!(
"{}, {}",
String::from_utf8(key.to_vec()).map_err(|e| { ExecuteCommandError::Utf8Error(e.utf8_error()) })?,
Copy link
Owner

Choose a reason for hiding this comment

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

You don't need to convert Bytes to String, cloning bytes doesnt' cause any allocations as it is zero copy. Converting string to bytes will cause unnecessary allocations. Our strings are binary safe, but if we convert binary data to utf_8 strings that might cause some issues.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good 👍

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.

None yet

2 participants