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

./rust code update and ./dss/raft description update #301

Merged
merged 6 commits into from Nov 19, 2019

Conversation

HunDunDM
Copy link
Contributor

@HunDunDM HunDunDM commented Oct 8, 2019

  • Practical Networked Applications in Rust
    • Update: BufReaderWithPos and BufWriterWithPos.
    • Fix: Temp_dir is dropped during the test.
  • Distributed Systems in Rust
    • Update: README.md
    • Fix: wrong code in the comment.

Because TempDir will delete the folder when it is in Drop.
If the file written in append mode is not an empty file, the return value of `seek(SeekFrom::Current(0))` is not the current write position but the beginning of the file before executing a `write`.
@CLAassistant
Copy link

CLAassistant commented Oct 8, 2019

CLA assistant check
All committers have signed the CLA.

@Hoverbear Hoverbear requested a review from brson October 8, 2019 23:02
Copy link
Member

@overvenus overvenus left a comment

Choose a reason for hiding this comment

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

Thanks!

@overvenus
Copy link
Member

PTAL @sticnarf

Copy link
Collaborator

@sticnarf sticnarf left a comment

Choose a reason for hiding this comment

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

Thanks! But I think when creating BufWriterWithPos and BufReaderWithPos, it shoudn't have extra semantics about where its cursor should be. SeekFrom::Current(0) is just to update the internal pos to be the same as the actual one.
The rest LGTM.

@HunDunDM
Copy link
Contributor Author

HunDunDM commented Nov 17, 2019

Thanks! But I think when creating BufWriterWithPos and BufReaderWithPos, it shoudn't have extra semantics about where its cursor should be. SeekFrom::Current(0) is just to update the internal pos to be the same as the actual one.
The rest LGTM.

I agree. So can I think that the return value of seek(SeekFrom::Current(0)) is inconsistent with expectations?

@sticnarf
Copy link
Collaborator

Thanks! But I think when creating BufWriterWithPos and BufReaderWithPos, it shoudn't have extra semantics about where its cursor should be. SeekFrom::Current(0) is just to update the internal pos to be the same as the actual one.
The rest LGTM.

I agree. So can I think that the return value of seek(SeekFrom::Current(0)) is inconsistent with expectations?

Why? The return value of seek(SeekFrom::Current(0))is right the current position of the stream.

@HunDunDM
Copy link
Contributor Author

HunDunDM commented Nov 18, 2019

Why? The return value of seek(SeekFrom::Current(0))is right the current position of the stream.

use std::fs;
use std::io::{BufWriter, Seek, SeekFrom, Write};
fn main() {
    let fs = fs::OpenOptions::new()
        .write(true)
        .append(true)
        .open("./temp")
        .unwrap(); // Non-empty file, assuming 5 bytes
    let mut buf = BufWriter::new(fs);
    let pos1 = buf.seek(SeekFrom::Current(0)).unwrap();
    let slice = vec![126];
    buf.write(&slice).unwrap();
    buf.flush().unwrap();
    let pos2 = buf.seek(SeekFrom::Current(0)).unwrap();
    println!("first pos: {}, next pos: {}.", pos1, pos2);
    // output: first pos: 0, next pos: 6.
}

In the implementation of my homework, I specified it externally. I set pos to seek(SeekFrom::End(0)) for File and 0 for TcpStream. Because there are some types that implements Write but does not implement Seek.

impl<W: Write> BufWriterWithPos<W> {
    pub fn new(inner: W, pos: u64) -> Self {
        Self {
            writer: BufWriter::new(inner),
            pos,
        }
    }
}

@sticnarf
Copy link
Collaborator

@HunDunDM I see. It's just implementation related. The reference implementation doesn't write to an existing file with content, so the position is always right. If I need to append to an existing file with content, I should seek manually to the end of the file then the position is correct. I think it's incorrect to seek to the end when initializing BufWriterWithPos because BufWriterWithPos should know nothing about the open mode.

@HunDunDM
Copy link
Contributor Author

@sticnarf You are right, it should be sought to the end before BufWriterWithPos::new.

I rolled back the relevant code.

Copy link
Collaborator

@sticnarf sticnarf left a comment

Choose a reason for hiding this comment

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

LGTM

@sticnarf sticnarf merged commit dbd7113 into pingcap:master Nov 19, 2019
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

4 participants