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

Porting prost migration to raft 0.4 #204

Merged
merged 14 commits into from
Apr 23, 2019
Merged

Porting prost migration to raft 0.4 #204

merged 14 commits into from
Apr 23, 2019

Conversation

ice1000
Copy link
Contributor

@ice1000 ice1000 commented Apr 4, 2019

For TiKV usages.
This is opened as a PR only for code review, please don't merge.

Currently there's one assertion error in tests but the production code are (seem to) working.

ice1000 and others added 5 commits April 4, 2019 17:04
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
* Migrate to Rust 2018

* Remove rust-toolchain file

The toolchains tested on CI are specified in their own ways, so this is only
used for local builds where developers. Its better for developers not to have
this file around in case they have their own preferences.

(cherry picked from commit addddd2)
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Copy link
Contributor Author

ice1000 commented Apr 4, 2019

@nrc
Copy link
Contributor

nrc commented Apr 5, 2019

The assertion error is in ...

On master this is assert_eq!(rs.index, 2);. I wonder if there is a rebasing error?

Copy link
Contributor

@nrc nrc left a comment

Choose a reason for hiding this comment

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

LGTM

Cargo.toml Outdated Show resolved Hide resolved
@nrc nrc requested a review from Hoverbear April 5, 2019 02:14
@Hoverbear
Copy link
Contributor

Seems it would be good if we made a branch off 0.4.1 and then merged this with that, then we can release 0.4.2 from that branch?

@ice1000
Copy link
Contributor Author

ice1000 commented Apr 9, 2019

@Hoverbear sounds good to me!

@Hoverbear Hoverbear changed the base branch from master to 0.4.1 April 10, 2019 17:08
@Hoverbear
Copy link
Contributor

Seems we never released 0.4.1, so we'll make this 0.4.1. :)

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.

Rest LGTM, please fix some of the nits though :)

Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Copy link
Contributor Author

ice1000 commented Apr 11, 2019

All comments addressed except those depends on tikv/protobuf-build#3.

Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@CLAassistant
Copy link

CLAassistant commented Apr 11, 2019

CLA assistant check
All committers have signed the CLA.

Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Copy link
Contributor Author

ice1000 commented Apr 11, 2019

@nrc PTAL at the CLA

@nrc
Copy link
Contributor

nrc commented Apr 11, 2019

I think this just needs the protobuf-build update and then can be merged

@ice1000
Copy link
Contributor Author

ice1000 commented Apr 11, 2019

Comments related to tikv/protobuf-build#3 are addressed.

@nrc
Copy link
Contributor

nrc commented Apr 12, 2019

Failing tests now :-(

@Hoverbear
Copy link
Contributor

Forcing a rebuild on CI

@ice1000 ice1000 changed the title [DNM] Porting prost migration to raft 0.4 Porting prost migration to raft 0.4 Apr 12, 2019
@ice1000
Copy link
Contributor Author

ice1000 commented Apr 12, 2019

I have one test failing locally but I didn't know why (at least seem to be unrelated to pb library).

Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Copy link
Contributor Author

ice1000 commented Apr 18, 2019

It's supposed to be fixed now. Let's wait for CI response...

@nrc
Copy link
Contributor

nrc commented Apr 18, 2019

Still some errors, looks like lints

src/errors.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@siddontang siddontang added the Do Not Merge A blocked PR. Please do not merge it. label Apr 18, 2019
@nrc
Copy link
Contributor

nrc commented Apr 19, 2019

@ice1000 can you pull in #219 to fix the CI please?

nrc and others added 2 commits April 18, 2019 23:50
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Copy link
Contributor Author

ice1000 commented Apr 19, 2019

All comments addressed. Mr. Travis CI, please start your performance

@ice1000
Copy link
Contributor Author

ice1000 commented Apr 19, 2019

error: the feature `tool_lints` has been stable since 1.31.0 and no longer requires an attribute to enable
   --> src/lib.rs:264:47
    |
264 | #![cfg_attr(feature = "cargo-clippy", feature(tool_lints))]
    |                                               ^^^^^^^^^^
    |
    = note: `-D stable-features` implied by `-D warnings`
error: this .into_iter() call is equivalent to .iter() and will not move the slice
   --> src/raft.rs:789:14
    |
789 |         ents.into_iter()
    |              ^^^^^^^^^ help: call directly: `iter`
    |
    = note: `-D clippy::into-iter-on-ref` implied by `-D clippy::all`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#into_iter_on_ref
error: aborting due to 2 previous errors

Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
src/lib.rs Outdated Show resolved Hide resolved
Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Copy link
Contributor Author

ice1000 commented Apr 19, 2019

I don't understand. I can see a lot of stacktraces in the nightly CI build, but it's saying test all passed.
Is that what we expect to see?

@ice1000
Copy link
Contributor Author

ice1000 commented Apr 19, 2019

Yay! CI passed!

@nrc
Copy link
Contributor

nrc commented Apr 19, 2019

@Hoverbear can we release this as 0.4.1 please?

@Hoverbear Hoverbear merged commit e6ba1a2 into tikv:0.4.1 Apr 23, 2019
@Hoverbear
Copy link
Contributor

Sorry about the delay @ice1000 :(

@Hoverbear Hoverbear added this to the 0.4.1 milestone Apr 23, 2019
@Hoverbear Hoverbear removed the Do Not Merge A blocked PR. Please do not merge it. label Apr 23, 2019
@ice1000
Copy link
Contributor Author

ice1000 commented Apr 23, 2019

No problem, I can totally understand that you people are busy last week!

@ice1000 ice1000 deleted the 0.4-prost branch April 23, 2019 11:34
@BusyJay
Copy link
Member

BusyJay commented Apr 28, 2019

This PR introduces API changes, and should be landed on 0.5.x instead of 0.4.x.

@Hoverbear
Copy link
Contributor

@BusyJay Prior to 1.0.0 authors are free to break API as they please. 0.5.0 contains other changes and is already released. It's incompatible with TiKV.

@ice1000
Copy link
Contributor Author

ice1000 commented Apr 28, 2019

@BusyJay We have another PR that does the same thing but based on the master branch instead of the 0.4.0 release tag.

@nrc
Copy link
Contributor

nrc commented Apr 29, 2019

Discussed on Slack, we agreed to yank 0.4.1 and use Prost via a Git dep for the time being

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

6 participants