[WIP] Rust course project 2 tests and example solution #33
Conversation
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
| failure = "0.1.5" | ||
| serde = { version = "1.0.89", features = ["derive"] } |
There was a problem hiding this comment.
I did not know you could get the derive macros directly from the serde crate. Neat!
| for set_cmd in stream { | ||
| let set_cmd = set_cmd?; | ||
| map.insert(set_cmd.key, set_cmd.value); | ||
| } |
There was a problem hiding this comment.
I'm glad this code was so easy. I was afraid serde wouldn't deserialize multiple records in sequence like that.
rust/projects/file-io/tests/tests.rs
Outdated
| temp_dir.push(name); | ||
| create_dir_all(&temp_dir).expect("unable to create working directory"); | ||
| temp_dir | ||
| } |
There was a problem hiding this comment.
From the looks of this the temp dirs are not deleted up after the test. Can you instead use the tempfile crate's tempdir, which has destructors that automatically clean up.
There was a problem hiding this comment.
I was avoiding introducing dependencies to tests. If we do so, we'll either change to use a workspace or just add dev-dependencies to Cargo.toml. I prefer the latter one. I think we can add necessary dev-dependencies to the Cargo.toml at the beginning of the course in project 1 and tell them these dependencies are only used for tests.
There was a problem hiding this comment.
This way, students copy the content of Cargo.toml at first, and copy the tests of each project, then everything will go fine.
rust/projects/file-io/src/kv.rs
Outdated
| } | ||
|
|
||
| /// Sets the value of a string key to a string. | ||
| /// | ||
| /// If the key already exists, the previous value will be overwritten. | ||
| pub fn set(&mut self, key: String, value: String) -> Result<()> { | ||
| self.map.insert(key, value); | ||
| let cmd = SetCommand::new(key, value); | ||
| to_writer(&mut self.wal_writer, &cmd)?; |
There was a problem hiding this comment.
It took me a few seconds to figure out that to_writer isn't in this crate, but from serde_json.
It's common in Rust to import types, but to call functions through their module name, like serde_json::to_writer. Will you do that here for clarity?
There was a problem hiding this comment.
Great stuff again.
I put a few change requests inline.
The use of BufWriter is interesting. Here's the commit code as written:
let cmd = SetCommand::new(key, value);
to_writer(&mut self.wal_writer, &cmd)?;
self.map.insert(cmd.key, cmd.value);
Ok(())
The function returns to the caller with Ok. That means that their data is definitely inserted. So even if there's a crash they can restart and see the commit. To guarantee that, the userspace buffer and the kernel buffer need to be flushed to disk prior to inserting into the map. The project hasn't covered that yet, but I think it needs to.
If the data is immediately flushed then is the BufWriter doing something useful? I think it depends on how serde treats the writer, whether it writes in small increments, or whether it writes in big chunks.
Probably it writes in small pieces and you are right to use the BufWriter. But I don't know.
That's all I've got. Good stuff.
|
@brson After some investigation, I confirm serde_json writes in small pieces, so a You're right. To ensure we do not lose data in the buffer, we need a flush on every write of the user. |
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
|
Thanks @sticnarf. I went ahead and merged. Please continue the project in a new branch. |
No description provided.