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

Publishing? #280

Open
Hoverbear opened this issue Jun 21, 2018 · 31 comments
Open

Publishing? #280

Hoverbear opened this issue Jun 21, 2018 · 31 comments
Labels

Comments

@Hoverbear
Copy link
Contributor

Hoverbear commented Jun 21, 2018

I'm interested in publishing this crate so that it can be used as part of the upcoming TiKV client in Rust (which will be published on crates.io).

This already builds cleanly on nightly and has no illegal dependencies, so there should be nothing blocking publishing this.

I would recommend renaming this crate to tikv_proto though, so in the future tikv_client and tikv_server will be consistent.

@Hoverbear
Copy link
Contributor Author

@siddontang PTAL

@Hoverbear
Copy link
Contributor Author

We need to seriously consider this with the tikv-client for Rust. We cannot publish the client without all of its dependencies also being published.

@nrc
Copy link
Contributor

nrc commented May 20, 2019

Some things we need to do/think about:

  • Use a crates.io version of Raft (I think this just needs us to finish the Prost migration and other Raft versioning issues.)
  • Consider the name - 'kvproto' is not very self-explanatory (tikv_proto is suggested above, I prefer tikv-proto (consistent with rust-client, which should probably be tikv-client.rs or something)
  • Consider and document the process for releases
  • What should a Rust crate look like? We probably should not include the Go and CPP code in the distributed tarball, for example.
  • building - are we happy with the current process and requirements for building (see also Installation requires cmake, protoc, golang, perl... #354)
  • think about the project org maybe? (Some things worth considering: why is Raft the source of truth for it's Protos, but this project is for the other protos? Why would a proto be here vs elsewhere? Should we be committing the generated code? Is having a multi-language project the right approach (especially if we add more languages in the future)?
  • should the repo live in the TiKV org?

Probably more...

@Hoverbear
Copy link
Contributor Author

@nrc Do you think extracting only the tikv related protos and making them a dependency here would be a good first step to enable this? Maybe a tikv-api crate? Then we could publish that.

@nrc
Copy link
Contributor

nrc commented May 20, 2019

Do you think extracting only the tikv related protos and making them a dependency here would be a good first step to enable this? Maybe a tikv-api crate?

I think that is probably a good idea. I wonder whether the Raft protos should be part of that?

@ice1000
Copy link
Contributor

ice1000 commented May 21, 2019

These protos were too big for Raft (which is relatively small) to depend on, but the situation may change after prost

@BusyJay
Copy link
Contributor

BusyJay commented May 21, 2019

No, raft is a standard library that should keep its own protocols.

@nrc
Copy link
Contributor

nrc commented May 21, 2019

I don't understand either of those points, sorry.

These protos were too big for Raft (which is relatively small) to depend on

Why is size an issue?

No, raft is a standard library that should keep its own protocols.

What do you mean by "standard" (and by implication why are TikV, etc., not standard?)

@BusyJay
Copy link
Contributor

BusyJay commented May 21, 2019

Sorry, it should be standalone. 🤣

@ice1000
Copy link
Contributor

ice1000 commented May 21, 2019

Why is size an issue?

Hmm, compiling kvproto takes a long time.

@nrc
Copy link
Contributor

nrc commented May 22, 2019

Hmm, compiling kvproto takes a long time.

This is true of a full build (1m 16s for me), but building kvproto itself only takes 7s. Given that any deps of kvproto should be required by Raft too, it is that which matters (in practice, I think we pull in grpc-rs, which we don't really need to, I'll file an issue about that).

@nrc
Copy link
Contributor

nrc commented May 22, 2019

Sorry, it should be standalone. 🤣

That makes more sense, thanks! However, I'm still a bit unclear why Raft is a standalone library in this sense. If it is meant to be self-contained, why do we need to re-export the proto interface here?

@BusyJay
Copy link
Contributor

BusyJay commented May 22, 2019

...why do we need to re-export the proto interface here?

Because kvproto uses the struct of raft protocol directly, and due to the limited of protobuf compiler, we need to reexport raft proto interface to make the generated code compiled.

@nrc
Copy link
Contributor

nrc commented May 22, 2019

Because kvproto uses the struct of raft protocol directly, and due to the limited of protobuf compiler, we need to reexport raft proto interface to make the generated code compiled.

Cool, thanks!

@nrc
Copy link
Contributor

nrc commented Jun 4, 2019

Proposal:

  • Move the Raft protos into a RaftProto repo
  • Raft depends on RaftProto
  • KvProto depends on RaftProto, not Raft
  • Move the code for generating protos in Go into protobuf-build so that KvProto and RaftProto can share that code rather than duplicate it
  • Publish RaftProto and KvProto on crates.io

How does that sound? @BusyJay @Hoverbear

@BusyJay
Copy link
Contributor

BusyJay commented Jun 5, 2019

It sounds good to me. But I have also been thinking about removing raft dependency from kvproto, so that raft is free to change its protocols, or even no protocols at all.

/cc @siddontang what do you think?

@ice1000
Copy link
Contributor

ice1000 commented Jun 5, 2019

so that raft is free to change its protocols, or even no protocols at all.

I suppose raft and kvproto will depend on the same "raftproto" library mentioned in @nrc's comment

@Hoverbear
Copy link
Contributor Author

@nrc I approve of this idea!

@nrc
Copy link
Contributor

nrc commented Jun 14, 2019

It sounds good to me. But I have also been thinking about removing raft dependency from kvproto, so that raft is free to change its protocols, or even no protocols at all.

@BusyJay @siddontang do you have any more thoughts on this?

@BusyJay
Copy link
Contributor

BusyJay commented Jun 17, 2019

I just take a look into the protocols, it seems there are many places using eraft protos. It's not practical to remove the dependency now. So your proposal seems the only way to publish.

@nrc
Copy link
Contributor

nrc commented Jun 17, 2019

Cool, thanks for looking at the protocols!

nrc added a commit to nrc/kvproto that referenced this issue Jun 18, 2019
We now only depend on crates.io crates, cc pingcap#280

Signed-off-by: Nick Cameron <nrc@ncameron.org>
nrc added a commit to nrc/kvproto that referenced this issue Jun 19, 2019
We now only depend on crates.io crates, cc pingcap#280

Signed-off-by: Nick Cameron <nrc@ncameron.org>
nrc added a commit to nrc/kvproto that referenced this issue Jun 20, 2019
We now only depend on crates.io crates, cc pingcap#280

Signed-off-by: Nick Cameron <nrc@ncameron.org>
nrc added a commit that referenced this issue Jun 26, 2019
* Use the raft-proto crate instead of raft

We now only depend on crates.io crates, cc #280

Signed-off-by: Nick Cameron <nrc@ncameron.org>

* Update protobuf-build

Fixes a bunch of warnings

Signed-off-by: Nick Cameron <nrc@ncameron.org>
@Daniel-B-Smith
Copy link

Any update on publishing the crate?

@Hoverbear
Copy link
Contributor Author

@Daniel-B-Smith We're currently not able to publish this due to some ongoing work by @nrc . We're planning to publish this crate (along with our Rust client) in Q4. Sorry for the delays.

kvproto/Cargo.toml

Lines 31 to 32 in 121d118

"protobuf-codegen:2.8.0" = { git = "https://github.com/nrc/rust-protobuf", branch = "v2.8" }
"protobuf:2.8.0" = { git = "https://github.com/nrc/rust-protobuf", branch = "v2.8" }

@Daniel-B-Smith
Copy link

Thanks for the update!

sticnarf pushed a commit to sticnarf/kvproto that referenced this issue Oct 27, 2019
* Use the raft-proto crate instead of raft

We now only depend on crates.io crates, cc pingcap#280

Signed-off-by: Nick Cameron <nrc@ncameron.org>

* Update protobuf-build

Fixes a bunch of warnings

Signed-off-by: Nick Cameron <nrc@ncameron.org>
@xxchan
Copy link

xxchan commented Aug 23, 2021

Any update?

@disksing
Copy link
Contributor

disksing commented Aug 23, 2021

anyone know the status of this issue? cc @sticnarf @BusyJay

@BusyJay
Copy link
Contributor

BusyJay commented Aug 24, 2021

kvproto now uses forked rust-protobuf, I believe forked rust-protobuf needs to be published first.

@disksing
Copy link
Contributor

kvproto now uses forked rust-protobuf, I believe forked rust-protobuf needs to be published first.

How can we proceed? Do we currently have any plan for the next step?

@BusyJay
Copy link
Contributor

BusyJay commented Aug 24, 2021

As far as I know, no one is working on it.

1 similar comment
@BusyJay

This comment has been minimized.

@xxchan
Copy link

xxchan commented Aug 25, 2021

Maybe related post: Move kvproto to tikv org

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants