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

Add gRPC support #12

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add gRPC support #12

wants to merge 1 commit into from

Conversation

Johni0702
Copy link

This PR adds support for connecting to Murmur via gRPC instead of Ice.

Almost all of the Murmur Ice API is bridged to gRPC and existing modules do not need to be changed (and neither do future ones, they should continue to use the Ice API).

Modules that rely on specific exception being thrown by Ice might have a hard time because gRPC exceptions are mostly passed through the bridge instead of being transformed into their Ice counterparts. I'm not aware of any modules actually doing that though.

Tested with idlemove, onjoin, samplecontext, seen, (partially) source, setstatus and gmod.
Ice methods not used by any of these have been tested in the REPL but their return values might not be exactly what modules expect (I merely compared them to the docs, not to ones from an actual Ice client).

@hacst
Copy link
Contributor

hacst commented Jan 22, 2018

Hi. Cool patch. As it's pretty much an independent code-path I see no trouble landing this. Only thing I think needs a bit more love is the readme changes. Would be great if the setup section made clearer that grpc is an optional, "best-effort" alternative to Ice in mumo. As it is now I think some people might be confused about the difference. Maybe something in the direction of: "Besides Ice murmur also supports the alternative gRPC protocol. If using Ice is problematic in your environment gRPC offers you another way to access some (but not all) of mumo's functionality.". Bonus points for a gRPC restrictions section at the end with a short summary of what works and what not with a call to action in case someone wants to chip in to improve it.

Thanks again for the PR!

@raku-cat
Copy link
Contributor

This PR didn't actually work for me using python2.7(python3 didn't work at all, not unexpected, didn't try 2to3 though).
I had to move initdata.logger = CustomLogger() below the actual CustomLogger function else I got an UnboundLocalError.

@Kissaki Kissaki force-pushed the master branch 3 times, most recently from 7d19720 to 1f36911 Compare March 11, 2024 23:42
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.

3 participants