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

Reduce some internal panics. #129

Closed
Hoverbear opened this issue Oct 1, 2018 · 2 comments
Closed

Reduce some internal panics. #129

Hoverbear opened this issue Oct 1, 2018 · 2 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

The Raft library has several internal panics. This is partially due to being fairly old code with Go origins, and partially due to it being previously internal to TiKV.

This issue, broadly, invites you to tackle any panic!() or .unwrap() you find in the library.

Notable targets to get you started:

  • fn insert_learner in src/raft.rs.
  • fn insert_voter in src/raft.rs.

You are welcome to tackle any potential panics though. Please link them back to this issue for overall tracking.

Your PR may take some time to merge as we would like to stage public API changes. The next targeted API change is in 0.5.0.

@Hoverbear Hoverbear added Help Wanted An issue with unsolved problems, looking for help. Good First Issue A good issue for a new contributor. labels Oct 1, 2018
@JoshMcguigan
Copy link
Contributor

It looks like fn insert_learner and fn insert_voter both return a Result at this time. At a quick glance, the only way for them to panic would be for the assert_eq check in fn assert_progress_and_configuration_consistent to fail.

Did you want to add a ProgressConfigurationInconsistent error type and return that rather than panic? Or can this issue be marked as resolved for those two methods?

I'd be interested in taking a look at this issue, if you have any guidance on a particular panic! or unwrap which would be a good candidate for removal (assuming the two methods above should be marked as done).

@Hoverbear
Copy link
Contributor Author

Hi @JoshMcguigan You're right. I think this has been fixed, actually! :( Darnit!

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

2 participants