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

murmur grpc support #1682

Closed
wants to merge 179 commits into from

Conversation

@bontibon
Copy link
Contributor

commented May 25, 2015

This PR adds grpc support to murmur, as part of a Google Summer of Code project. The goal is to provide increased availability to murmur's RPC functionality to those who are unable to use Ice.

In order for this project to be successful, it is very important that you, the community, help in providing feedback to what I am working on. In the early stage of the project, this will be input on the structure of the protocol buffer file. For those of you who are familiar with Ice, the feel of grpc may take some getting used to. However, with your help, I'm hoping to make the API as natural as possible.

If this interests you, I would encourage you to subscribe to this PR to receive notifications about its development. Thanks!


TODO

  • Service implementations
    • ServerService
    • MetaService
    • ContextActionService
    • TextMessageService
    • LogService
    • ConfigService
    • ChannelService
    • UserService
    • TreeService
    • BanService
    • ACLService
    • AuthenticatorService
    • DatabaseService
    • AudioService
  • Documentation
  • Fix TODO items
  • Sample application (murmur-cli)
  • Extra features (outside of the proposed scope of the project)
    • TextMessageService.Filter.
    • UserService.Ban.
    • Include client IP address, session ID in Authenticator.Request.Authenticate.
    • Support setting textures via UserUpdate.
    • API authentication (access APIs with keys, or maybe certificates?).
    • Track user context actions to prevent users from triggering actions they have not explicitly been sent.
    • Have Authenticator.Request.Find provide a hint as to what information it is interested in.
    • Add method that returns the names of available configuration options.
    • Query redirect whisper groups.
    • Include timestamps in streaming events messages.
@mkrautz

This comment has been minimized.

Copy link
Member

commented May 25, 2015

Thanks Tim!

I'll make sure you get prompt feedback on anything I can help with.

@mkrautz

This comment has been minimized.

Copy link
Member

commented May 25, 2015

And if you're seeing long response times, please ping me/us.

@bontibon

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2015

I have pushed a first, rough draft of the protocol buffer spec. I tried a couple of different ways to expose the API, and, IMO, this is the least objectionable.

I would appreciate any comments, questions, or suggestions. In particular, I am not overly pleased with the current design of the Authenticator and AuthenticatorService; it can probably be improved upon.

@mkrautz

This comment has been minimized.

Copy link
Member

commented May 31, 2015

I looked through this, and nothing had me screaming out loud.

Now, I should preface that with the fact that I'm not well-versed in the RPC code in Murmur, nor in GRPC.

I have a few concerns already, though:

Why switch from proto3 to proto2?

While I like the static config message, I don't think that will do. I think we need a map (proto3?) for it to work well. Maybe both. I do prefer it, but I do also foresee problems with it, when we add new stuff.
For now, perhaps just add a seperate get/set method for configs? That'd do it for me, at least.

Are you writing a log/blog on your work? It'd be interesting to have something that dumps your thoughts during the design, etc. -- obviously, we're not your mentors in this project, which makes it a bit weird in the first place. But a log/blog or some documentation that we can use to follow the progress and thought process.

In addition to the previous point, writing docs for this would be very helpful -- also for people reviewing it. I'm thinking Sphinx or something -- whatever you prefer, really.
Perhaps keep it in a separate repo so we can follow the history?

Also, a repository of examples would also be very helpful. I'm sure you have sample code already, but if you kept that in a repo of its own, us reviewers can easily grab example code and play with the GRPC interface.

Thanks for the work so far!

@mkrautz

This comment has been minimized.

Copy link
Member

commented May 31, 2015

Just to add: @hacst reminds me that proto3 has no notion of optional fields that do not need to be set.

That probably explains it, I'm guessing. :-)

@mkrautz

This comment has been minimized.

Copy link
Member

commented May 31, 2015

Also,

With Ice, it's possible to fetch the slice file (protocol description file) from the server via Ice RPC.

Since GRPC is HTTP/2 based, something like that should, in theory, be possible to accomplish there as well -- just with regular HTTP semantics.

Is that possible with GRPC as well? If so, that might remove some of my fears for the config message. But I guess this isn't possible since GRPC requires you to precompile protos?

@bontibon

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2015

Thanks for your feedback, @mkrautz.

Why switch from proto3 to proto2?

The original commit you saw used proto3, but I have since reverted back to proto2. This was due to the issue you mentioned: lack of optional fields.

While I like the static config message, I don't think that will do. I think we need a map (proto3?) for it to work well. ... For now, perhaps just add a seperate get/set method for configs? That'd do it for me, at least.

I have not implemented the config service yet, so I'll just let it sit for now; I'll take a run at it if I think of an elegant-ish solution.

Also, proto2 supports maps, so we can use those if need be.

Are you writing a log/blog on your work?

Yup, you can find it here: http://summer.cs.pdx.edu/blog/4191.

writing docs for this would be very helpful

The RPC protocol buffer file is going to be documented as I implement the various services, similar to how the Ice file is documented. Additionally, any code that needs documentation will receive it as well.

One of the deliverables for the project is a "getting started" guide. That will most likely appear after the majority of the RPC has been implemented.

Also, a repository of examples would also be very helpful.

Agreed. I'll make that up this week.

Is [fetching a file from the server] possible with GRPC as well?

I believe that's in the works. See grpc/grpc#1807

@mkrautz

This comment has been minimized.

Copy link
Member

commented May 31, 2015

Thanks! Very helpful with a blog.

@hacst

This comment has been minimized.

Copy link
Member

commented May 31, 2015

Fetching the descriptor file alone would not be sufficient. The reason why we can work with dynamic slice files from say Python is that the Ice module has an API for the slice compiler built into it. For grpc you'd likely need protoc with the grpc module around.

As mentioned on IRC using proto2 has other advantages for us. If there's no additional magic to grpc messages it should be pretty easy to offer command channel injection/listening/filtering over the RPC if it uses the same protobuf version as our internal protocol. Not sure if having that as part of the public RPC is such a good idea but it would be a very useful testing and debugging tool for us.

@hacst

This comment has been minimized.

Copy link
Member

commented May 31, 2015

I'll have to do some more reading on GRPC before being able to meaningfully contribute to this issue. GRPC is something I had an eye on as a potential replacement for our command channel. Breaking protocol compatibility is quite some time away though so I didn't look to closely. To be honest I'm a bit surprised this project was launched before even consulting with us. While I do realize this is theoretically independent of us, having some basic discussions about scope, goals and roadmap beforehand would have made a lot of sense imho.

@bontibon

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2015

Another part of my Summer of Code project is to use the new RPC interface in an application. For this, I have started writing a tool for managing murmur via a command-line interface. You can follow its development here: https://github.com/layeh/murmur-cli

@mkrautz mkrautz self-assigned this Jul 10, 2015

@bontibon

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2015

The grpc library is ready for general testing. Please feel free to install grpc and try going through the sample application and report and bugs or issues.

Thanks.

@bontibon

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2015

ping @mkrautz

This is the last week that I'm officially working on the patch. It would be preferable that any needed changes happen before Friday (no pressure though!).

@mkrautz

This comment has been minimized.

Copy link
Member

commented Aug 17, 2015

Will do. 👍

@mkrautz

This comment has been minimized.

Copy link
Member

commented Aug 21, 2015

Sorry, I didn't make it. For now, I only have a few high level comments:

  • Since GRPC isn't available for all platforms, it should probably land as disabled by default for all platforms. So I think we want CONFIG+=grpc to enable it, instead of CONFIG+=no-grpc to disable it. At least at first.
  • When this is landed sometime in the future, we can't ship the murmur.ini changes you have done in here. (Since we will land it as disabled by default). Feel free to copy murmur.ini to murmur.ini.grpc and add the GRPC-specific variables to it so there is a reference.
  • The file at scripts/Makefile should probably be integrated into the build. I suppose this means it will need to move to src/murmur_grpc_wrapper_gen or something. This would be built before Murmur, and murmur.pro would use the binary. Why integrate it into the build? Well, once we need GRPC on Windows and OS X, the Makefile is no longer ideal. [Not sure this is a blocker yet!]
  • I like that it is very self-contained. That means the barrier to get merged is much lower.

That's all I have for now. And once again, sorry for not being able to review it properly before your deadline.
Love the work that has been done here!

@hacst

This comment has been minimized.

Copy link
Member

commented Oct 28, 2015

Google released gRPC beta (http://googledevelopers.blogspot.de/2015/10/grpc-releases-beta-opening-door-for-use.html). There are now debian stable/backport packages available and my Ubuntu 15.10 has it in the repository. Sounds like gRPC might be in a place where you can actually ship it to users soon.

@bontibon

This comment has been minimized.

Copy link
Contributor Author

commented Oct 28, 2015

Thanks for the update, @hacst. I'll try to make my PR mergeable as soon as possible.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Oct 29, 2015

@bontibon I am not sure the PR is stalled at you, as much as it is awaiting a proper review from me/us. Sorry about that.

In general terms, I think the code looks good, and is very self-contained, so I see no large issues merging it. But I still haven't sat down and read all through it carefully. I would like to do that before we merge.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Mar 11, 2016

I spent some time working on this some time ago:
https://github.com/mkrautz/mumble/commits/grpc

@bontibon had any time to look at GRPC lately? I'd like to include it experimentally in our source tarballs for 1.3.0.

@mkrautz mkrautz added this to the 1.3.0 milestone Mar 11, 2016

mkrautz and others added 26 commits Feb 20, 2016
grpc: rename murmur_grpc_wrapper_gen to murmur_grpcwrapper_protoc_plu…
…gin.

Also, rename the binary to protoc-plugin-murmur-grpcwrapper.
grpc: fix Server::sendTextMessageGRPC and Server::setChannelState to …
…build in non-C++11 mode.

Before C++11, <:: is expanded as a trigraph in this context.
@bontibon

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2016

@mkrautz The crash you reported should be fixed with 6a0a322.

mkrautz added a commit that referenced this pull request May 8, 2016
@mkrautz

This comment has been minimized.

Copy link
Member

commented May 8, 2016

Landed. Finally! 👊

Thanks a lot @bontibon

@mkrautz mkrautz closed this May 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.