-
Notifications
You must be signed in to change notification settings - Fork 238
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
L4: Add JRuby Support #13
Conversation
Yes please. Many rubyists choose JRuby for their production environment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICS the main difficulty here is in matching the high-level API of C-wrapping grpc-ruby with a ruby shim over the java-client. The main issues coming down the the differences in API and behavior of the grpc C-core vs. grpc-java libraries.
L4.md
Outdated
|
||
1. Update the Gemspec to add JBundler to the list of development dependencies when run on JRuby | ||
1. Create a [JarFile](https://github.com/torquebox/maven-tools/wiki/Jarfile) to create a dependency on the grpc-java jar | ||
1. Create wrapper code that delegates the existing Ruby API calls to the Java client, as needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A heads up about this, I would expect this shim layer over the java client to end up as about a full grpc-ruby reimplementation, with the requirement that it always present the same surface as the C-wrapping grpc-ruby gem. The majority of the current grpc-ruby code base is probably not immediately reusable - the pure ruby part is thin and a lot of it is tied to the C-extension ruby API, of which the semantics revolve around the grpc C-core library interface. Also the grpc-java libraries present slightly higher level and slightly different interfaces than the grpc C-core library that the ruby C-extensions wrap. For example, one trouble point is "channel args". Currently, users can pass a hash to grpc-ruby Channel
constructors, of which the key-value pairs are passed straight down to the grpc C-core library - emulating this on top of grpc-java client, whose implementation does need to support these in the same way, would probably be difficult if doable at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the assumption correct that there was and continues to be either a performance reason and / or an ongoing maintenance reason not to build a pure Ruby implementation? If that assumption holds, do we agree that solutions that build on top of either the C client or the Java client are the only viable ways forward?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but just making clear that the grpc-java to grpc-ruby shim would be involved, with API copies probably not feasible. The other option I can see is a JNI wrapper of the C-core library, which would not be small task either but could be useful for other languages outside jruby..
L4.md
Outdated
1. Update the Gemspec to add JBundler to the list of development dependencies when run on JRuby | ||
1. Create a [JarFile](https://github.com/torquebox/maven-tools/wiki/Jarfile) to create a dependency on the grpc-java jar | ||
1. Create wrapper code that delegates the existing Ruby API calls to the Java client, as needed | ||
1. Address any test regressions or expectation updates introduced by executing on JRuby |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably wouldn't be able to drop the jruby target into the current tests without changes to the tests themselves. This is probably doable but somewhat involved.
The testing suite would have to be updated to add "jruby", as basically a separate language, to the continuously tested language-platform matrix. Also I suspect a lot of the current grpc-ruby tests would have to be refactored to be agnostic of the C-wrapping and java-wrapping implementations (higher level tests should work find but there's lower level stuff in C-extensions is used in tests).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it fair to say that the high level tests are all that we care about in the context of JRuby because low-level tests will be exercised by unit tests with grpc-java
, and that the rest could be marked to be skipped when running on JRuby?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the most part, yeah I think so.
Should we move discussion to a Google groups thread? I think that phase is supposed to happen after a reviewer is assigned - @apolcyn, should I update the PR to reflect you as a reviewer? |
yeah @JasonLunn we can start a thread on https://groups.google.com/forum/#!forum/grpc-io @ejona86 looped you in on this, for the question of writing on top of grpc-java |
Updated discussion link to point at the newly created topic in the Google Group
Discussion is up there by the way: https://groups.google.com/forum/#!topic/grpc-io/4P6jg4ZYtfo |
Closing pull request due to inactivity. Feel free to re-open the comments and revive the PR when appropriate. |
@hsaliak - I don't understand what your looking to "revive" this PR? I believe that I've replied to all the feedback that I've received on the discussion group thread. I replied to the last comment directly to the poster ( @nicolasnoble ) rather than the thread itself, but didn't receive a reply. |
Ah I did not know that there was a private thread. The public thread ended with a ping with no response. |
Additional details requested in the news group
@hsaliak - radio silence from @nicolasnoble since the exchange that I referenced in April. |
I am going to close this PR for now, as the discussion thread had a few unanswered questions and is now stale.. |
Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement. After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.
Regards, |
@JasonLunn @hsaliak @apolcyn @ejona86 |
Is there any interest in making this happen at this point? We need this to be able to use google's published gems. I recently reintroduced jruby support to the protobuf gem: protocolbuffers/protobuf#7923 but without grpc support, it doesn't help too much. |
@rdubya - I'd be interested in reviving this effort, but having been based on the state-of-the-art of 2017, it's probably best if we start over with a fresh proposal. If you're interested in having a collaborator, my contact info is in my GH profile. |
Hey @JasonLunn, I'll just be picking at this when I have some spare moments (which are few these days unfortunately.) I figured I'd go the approach that the protobuf gem took and wrap the java version's functionality in a ruby/java wrapper. Even if it doesn't get pulled into this repo, at least there will be a version that can be referenced. |
No description provided.