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

WIP: JRuby support #106

Closed
wants to merge 21 commits into from
Closed

Conversation

Adithya-copart
Copy link
Contributor

@Adithya-copart Adithya-copart commented Jan 16, 2020

Thanks for the library.

I came across karafka/karafka#433 (comment) and assumed that the maintainers would be willing to support JRuby as rdkafka-ruby appears to be the future backend for gems like karafka, racecar etc.

The changes in this PR include:

  1. Work-arounds for the lack of read_int64, read(:pointer), read_string_to_null.
  2. Work-around for Pointer#autorelease=.
  3. Tweaking the timeout in producer_spec to avoid intermittent failures.
  4. Use *_ptrptr for references to MemoryPointer to match with https://github.com/appsignal/rdkafka-ruby/blob/d0d71f669daadbfeec056085b8abe065478be784/lib/rdkafka/consumer/headers.rb#L13

The lack of these methods probably could be fixed by jruby/jruby#5947.

Pointer#autorelease= is not available which was introduced in #73. I tried to manually free them within the ensure blocks.

I'm don't think this is necessary as the *_ptrptr and *_ptr appear to be freed by the time ensure blocks are being run.

Free ptr objects in ensure blocks and add workaround for lack of read(:pointer), read_string_to_null and read_int64 ffi methods in JRuby
@Adithya-copart Adithya-copart marked this pull request as ready for review January 16, 2020 20:10
@Adithya-copart
Copy link
Contributor Author

Adithya-copart commented Jan 17, 2020

I don't think the spec matches the description here.
https://github.com/appsignal/rdkafka-ruby/blob/d0d71f669daadbfeec056085b8abe065478be784/spec/rdkafka/error_spec.rb#L73-L83

It should be expect(subject).not_to eq to fit the description but they are passing as-is in master and are failing consistently in JRuby. Would we need something like:

def ==(error)
   error.is_a?(self.class) || self.to_s == error.to_s
end

This did not fail in my local. I will try a few sample runs and see if it'll be consistent.

@mensfeld
Copy link
Member

Wow @Adithya-copart - I'm the creator of Karafka and yes, once we switch Karafka to librdkafka, JRuby and Truffle Ruby support will be restored. Your changes are a huge step towards that!

FYI Waterdrop is already ported to librdkafka!

@thijsc thijsc self-assigned this Jan 17, 2020
@thijsc
Copy link
Collaborator

thijsc commented Jan 17, 2020

Thanks for your contribution!

jRuby support wasn't initially a goal of this project. When running on the JVM I don't think it makes sense to use librdkafka. The Kafka Java client library seems like a more natural and probably more performant fit.

In an ideal world I think there should be another gem that's a great wrapper around the latest version of the Java client and one could swap them in or out. But that's a lot of work so I think making the FFI setup compatible with jRuby is probably the best way forward. I'll take a look at the code shortly.

@mensfeld
Copy link
Member

jRuby support wasn't initially a goal of this project. When running on the JVM I don't think it makes sense to use librdkafka. The Kafka Java client library seems like a more natural and probably more performant fit.

But you have to build up the whole ecosystem yourself. That is you will have to use the native Java stuff and rediscover them in the Ruby context. With those changes, you will soon get Karafka running with librdkafka underneath that could run with jruby and truffle for free.

@thijsc
Copy link
Collaborator

thijsc commented Jan 17, 2020

But you have to build up the whole ecosystem yourself. That is you will have to use the native Java stuff and rediscover them in the Ruby context. With those changes, you will soon get Karafka running with librdkafka underneath that could run with jruby and truffle for free.

I agree that this is the most pragmatic way forward. But viewed from a purely technical standpoint a Java wrapper would be the best the choice in my opinion.

@Adithya-copart
Copy link
Contributor Author

You both made good points. The costs associated with using native Java client in JRuby are not justifiable for us at the moment. It makes sense to add the dependencies for librdkafka and reap the benefits of existing Ruby libraries.

@mensfeld I did not look deep into karafka source code but looks like karafka leverages process isolation for consumer isolation.

Is the consumer_group tightly coupled to the process or the consumer class itself?

The costs associated with spawning a separate JVM per consumer group may not be scalable for JRuby and might be better to run multiple threads in the JVM. It might add the possibility of running multiple consumers in different consumer groups within the same process where using a single consumer to subscribe to multiple topics is not feasible.

I'm confident that this approach may not fit in libraries like karafka itself where majority of users run MRI Ruby. I just want to hear your thoughts on the approach.

@mensfeld
Copy link
Member

@Adithya-copart I don't want to go offtop here about Karafka so I opened an issue with your question: karafka/karafka#570

@thijsc
Copy link
Collaborator

thijsc commented Jan 24, 2020

This pull is also blocked on #98 by the way, if anybody has any fresh thoughts on that I'd love to hear them.

@Adithya-copart Adithya-copart changed the title JRuby support WIP: JRuby support Jan 27, 2020
@Adithya-copart
Copy link
Contributor Author

E, [2020-01-29T06:17:58.952506 #9416] ERROR -- : rdkafka: [thrd:GroupCoordinator]: 1/1 brokers are down

log writing failed. can't be called from trap context

@thijsc In my local, these error messages always follow a closed socket warning in the Kafka logs.

I had one of the forked process SegFault while calling the finalizer on a AutoPointer object.
Do you think using fork could trigger the finalizer in both child and parent processes?

@Adithya-copart Adithya-copart mentioned this pull request Jan 29, 2020
@Adithya-copart
Copy link
Contributor Author

Adithya-copart commented Jan 31, 2020

The tests are green in all 4 but travis encountered a SegFault in MRI builds after final GC.

/home/travis/.travis/functions: line 109:  9338 Segmentation fault      (core dumped) bundle exec rspec --format documentation

When converting a Rdkafka::TopicPartitionList to a native type, Rdkafka
was depending on AutoPointer to eventually free them. There are a couple
cases where a Client handle returns a pointer to the application that
the application is then required to free, which could be leading to test
instability.

This takes a pass at ensuring all uses of native TopicPartitionList
instances (even those return) are deterministically freed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants