search

Hoverbear / old-raft-rs

[Incomplete] A Raft implementation in Rust
https://hoverbear.github.io/raft-rs/raft/
MIT License
266 stars 41 forks source link

misc cleanup #111

Closed tbg closed 8 years ago

tbg commented 8 years ago

for #101. @danburkert suggested that while the log interface is best driven by the first non-trivial implementations in the works, it still makes sense now to have entry() with an owned argument.

tbg commented 8 years ago

I will back out the second commit (not sure what I was doing) but will leave it in for now since I commented on it, which GitHub sometimes hides after removing. original second commit is gone.

tbg commented 8 years ago

after a chat with @danburkert I've backed out the change which returned owned entries from entry (the standard user will use his WAL log implementation, and that can use the existing interface just fine thanks to mmap). What's left of the PR is misc refactoring, in particular providing entries(lo,hi) via the PersistentLog trait and simplifying some code.

tbg commented 8 years ago

ok, I think I've addressed the TODOs. The changes here are mostly cosmetic, but (in my opinion) net positive. It would be more interesting to get rid of the ad-hoc vector construction (now in log.entries(), previously in I think two places) since really all we need is a borrow to construct capnp messages from - doesn't seem hard but not worth it until the WAL log is done.

Hoverbear commented 8 years ago

Looks like aster broke... I'll refire.

Hoverbear commented 8 years ago

Yay, it works! @danburkert does this look good to you? You had some concerns earlier and I'm not sure they were addressed, but it looks like it.

danburkert commented 8 years ago

Looks good to me. Thanks @tschottdorf. @homu r+

homu commented 8 years ago

:pushpin: Commit b1dd39e has been approved by danburkert

homu commented 8 years ago

:hourglass: Testing commit b1dd39e with merge 456e1e6...

homu commented 8 years ago

:sunny: Test successful - status