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

Module shuffling and organizing #125

Open
Hoverbear opened this issue Sep 18, 2018 · 7 comments
Open

Module shuffling and organizing #125

Hoverbear opened this issue Sep 18, 2018 · 7 comments
Labels
Good First Issue A good issue for a new contributor. Help Wanted An issue with unsolved problems, looking for help.

Comments

@Hoverbear
Copy link
Contributor

Reorganize some of the modules to have their own files and directories.

In general:

  • Files should contain no more than one logical unit. (Eg. the RawNode struct and implementations.)
  • Files should contain in order:
    • Module level docs (if existing)
    • use statements.
    • Any constants. (Eg INVALID_ID)
    • Any short utility functions that are not associated with the structure.
    • Any minor structures/enums that are trivial and don't warrant their own file. Eg enum Foo { Bar, Baz }. (If it has implementations beyond 1-2 functions split it off)
    • The structure itself.
    • Any implementations for the structure.
    • A test module if it's small (less than 5 tests, otherwise make it a $NAME/{mod, tests}.rs pair).

Changes for this PR should only include moving code and updating related use statements etc. No logic changes should happen here, they deserve their own PR/issue. This is to ease review.

Rationale

In TiKV we've been seeing a lot of long build times and one optimization to this is splitting up large files. Since raft is a considerably smaller crate it's easy to test our ideas here. At the same time, Raft has some very large files which may be both intimidating and confusing to navigate for potential contributors.

Benefits

  • No run time performance changes are expected.
  • Build time performance should strictly improve.
  • Increased ease of navigation

Process

In some cases, we should to split files because they hold large structure definitions that can be split up.
For example, src/progress.rs contains the structs Progress and ProgressSet , as well as Inflights.

Split modules with several non-trivial structures. In our example, src/progress.rs becomes src/progress/{mod, progress, progress_set, inflights}.rs

There are also modules that are very large due to large test suites etc. They can be split up. For example, src/raft.rs contains a large #[test] module.

Extract large test suites into their own files. In our example all of the #[test] code can go in a tests file. src/raft.rs becomes src/raft/{mod, tests}.rs.

@Hoverbear Hoverbear added Help Wanted An issue with unsolved problems, looking for help. Good First Issue A good issue for a new contributor. Request for Comment A proposal to be considered. Analogous to an RFC in TiKV/Rust. labels Sep 18, 2018
@Hoverbear
Copy link
Contributor Author

Hoverbear commented Sep 18, 2018

@breeswish I know you looked at build performance on TiKV, maybe you can have a look and give a LGTM if you think it's a good thing to do?

@breezewish
Copy link
Member

I like this proposal! For TiKV, currently one of the slow cause might be rust-rocksdb, which requires a full fresh build of rocksdb and spend a lot of time.

@Hoverbear
Copy link
Contributor Author

Hoverbear commented Oct 2, 2018

@breeswish Okay. :) I'll start to migrate Raft-rs in this direction over time. Other PRs are welcome to contribute to this!

@Hoverbear Hoverbear removed the Request for Comment A proposal to be considered. Analogous to an RFC in TiKV/Rust. label Oct 2, 2018
@Hoverbear Hoverbear pinned this issue Feb 14, 2019
@Fullstop000
Copy link
Member

@Hoverbear I'll start to spilt src/progress.rs into mods after 0.5.0 is released. And this will also be a preparing for Follower Replication implementation.

@Hoverbear
Copy link
Contributor Author

Hoverbear commented Feb 15, 2019

@Fullstop000 That's great! Thanks!

Do you think we can write up some technical details about Follower Replication? I think it's deserving of a full TiKV RFC (https://github.com/tikv/rfcs/). I'd be happy to help you with that. Maybe we can connect on wechat?

image

@Fullstop000
Copy link
Member

Oh it's so cool having a chance to propose a RFC about it. But I'm still considering about details of the implementation, maybe we can discuss this later.

@Hoverbear
Copy link
Contributor Author

@Fullstop000 It is very exciting! Why don't you let me know when you've considered and we can set up a call to discuss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue A good issue for a new contributor. Help Wanted An issue with unsolved problems, looking for help.
Projects
None yet
Development

No branches or pull requests

3 participants