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

L4: Add JRuby Support #13

Closed
wants to merge 3 commits into from
Closed

L4: Add JRuby Support #13

wants to merge 3 commits into from

Conversation

JasonLunn
Copy link

No description provided.

@blowmage
Copy link

Yes please. Many rubyists choose JRuby for their production environment.

Copy link
Contributor

@apolcyn apolcyn left a 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
Copy link
Contributor

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.

Copy link
Author

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?

Copy link
Contributor

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
Copy link
Contributor

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).

Copy link
Author

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?

Copy link
Contributor

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.

@JasonLunn
Copy link
Author

JasonLunn commented Feb 14, 2017

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?

@apolcyn apolcyn self-assigned this Feb 14, 2017
@apolcyn
Copy link
Contributor

apolcyn commented Feb 14, 2017

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

@apolcyn apolcyn requested a review from ejona86 February 14, 2017 22:06
Updated discussion link to point at the newly created topic in the Google Group
@nicolasnoble
Copy link
Member

Discussion is up there by the way: https://groups.google.com/forum/#!topic/grpc-io/4P6jg4ZYtfo

@ctiller ctiller changed the title Add JRuby Support L4: Add JRuby Support Mar 31, 2017
@hsaliak
Copy link
Contributor

hsaliak commented Apr 14, 2017

Closing pull request due to inactivity. Feel free to re-open the comments and revive the PR when appropriate.

@hsaliak hsaliak closed this Apr 14, 2017
@JasonLunn
Copy link
Author

@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.

@hsaliak
Copy link
Contributor

hsaliak commented Apr 15, 2017

Ah I did not know that there was a private thread. The public thread ended with a ping with no response.

@hsaliak hsaliak reopened this Apr 15, 2017
Additional details requested in the news group
@JasonLunn
Copy link
Author

@hsaliak - radio silence from @nicolasnoble since the exchange that I referenced in April.
@apolcyn - From your perspective, what's needed before this PR can be accepted?

@hsaliak
Copy link
Contributor

hsaliak commented Feb 2, 2018

I am going to close this PR for now, as the discussion thread had a few unanswered questions and is now stale..

@hsaliak hsaliak closed this Feb 2, 2018
@thelinuxfoundation
Copy link

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,
CLA GitHub bot

@Legogris
Copy link

@JasonLunn @hsaliak @apolcyn @ejona86
I see this is long-dead - are there any blockers to consider this for merging or is it just a matter of CLA? Getting JRuby support is quite significant.

@rdubya
Copy link

rdubya commented Feb 5, 2021

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.

@JasonLunn
Copy link
Author

@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.

@rdubya
Copy link

rdubya commented Feb 8, 2021

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.

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

Successfully merging this pull request may close these issues.

None yet

9 participants