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

Change return type of Raft::new() to Result #122

Merged
merged 6 commits into from
Oct 2, 2018
Merged

Change return type of Raft::new() to Result #122

merged 6 commits into from
Oct 2, 2018

Conversation

kleimkuhler
Copy link
Contributor

This allows a ConfigInvalid error to propogate up and let user handle it.

Fixes #94

@DorianZheng
Copy link

@Hoverbear @BusyJay PTAL

@Hoverbear Hoverbear added this to the 0.5.0 milestone Sep 12, 2018
@Hoverbear Hoverbear added Enhancement An improvement to existing code. Do Not Merge A blocked PR. Please do not merge it. labels Sep 12, 2018
@Hoverbear
Copy link
Contributor

Hi @kleimkuhler, thanks for this! I'm scheduling this for 0.5.0 so it's got a DNM for the moment.

src/raft.rs Outdated Show resolved Hide resolved
@kleimkuhler
Copy link
Contributor Author

@Hoverbear Added the additional ? and made the test more explicit.

Seems this is gaining some additional "Merge branch ..." commits. I can squash everything down if you'd like; I'm not sure how clean your team likes to keep the git history!

Hoverbear
Hoverbear previously approved these changes Sep 17, 2018
Copy link
Contributor

@Hoverbear Hoverbear left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for this!

@Hoverbear
Copy link
Contributor

@kleimkuhler Whoops! We both forgot the benchmarks! Could you please bump those too?

@kleimkuhler
Copy link
Contributor Author

@Hoverbear Totally missed the benchmarking; sorry about that! Everything should be good to go now.

Hoverbear
Hoverbear previously approved these changes Sep 19, 2018
Copy link
Contributor

@Hoverbear Hoverbear left a comment

Choose a reason for hiding this comment

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

LGTM thank you. :)

@Hoverbear Hoverbear removed the Do Not Merge A blocked PR. Please do not merge it. label Sep 21, 2018
@hicqu
Copy link
Contributor

hicqu commented Sep 29, 2018

LGTM.

@Hoverbear
Copy link
Contributor

Thanks a lot for your work on this @kleimkuhler !

@Hoverbear Hoverbear merged commit 6f88eb6 into tikv:master Oct 2, 2018
@kleimkuhler kleimkuhler deleted the bad-raft-config-errors branch October 29, 2018 04:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement An improvement to existing code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants