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

New kafka #103

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

New kafka #103

wants to merge 6 commits into from

Conversation

brycemcd
Copy link

The current dependency in this gem to support Kafka is no longer supported. This was brought up in #100 .

This PR updates to a supported version of Kafka in ruby.

I've done some non-standard things in this PR to maintain backwards compatibility in order to avoid a major version bump and avoid falling into a backwards compatibility nightmare.

That said, a future version of this gem should be planned that completely removes the old Kafka implementation. I did my best to segment the code in such a way that the removal is straightforward.

As a bonus (and because of my own needs) the Kafka device will also support a TLS connection.

@codecov-io
Copy link

codecov-io commented Oct 13, 2016

Current coverage is 91.61% (diff: 96.31%)

No coverage report found for master at ae6cd48.

Powered by Codecov. Last update ae6cd48...f0a3c7c

@brycemcd
Copy link
Author

@dwbutler looks like CI is failing for two reasons:

  1. ruby version ruby-kafka requires Ruby version >= 2.1.0
  2. Trying to make a tls connection

I'll fix 2, but looks like 1 might be a perpetual problem. How do you want to handle that?

@dwbutler
Copy link
Owner

@brycemcd Thanks! I opened an issue with Ruby-Kafka to find out why they are so restrictive with the Ruby version.

@dwbutler
Copy link
Owner

Creating a new type of device is a good approach, since the configuration interface is different and Ruby-Kafka also doesn't support older versions of Ruby. The Poseidon device can be deprecated and removed if/when LogStashLogger ever hits 1.0.

This should be a minor version bump rather than a teeny bump because it introduces a new feature, rather than a bug fix.

@dwbutler
Copy link
Owner

Also, I would prefer to name the new device kafka and the old one poseidon for clarity. It's a breaking change but easily documented in the readme.

@dwbutler
Copy link
Owner

I verified that Ruby-Kafka indeed requires Ruby 2.1 and up. This is fine, it just needs to be documented in the readme, and the associated tests should be skipped for Ruby < 2.1.

@brycemcd
Copy link
Author

@dwbutler thanks for following up. I'll provide an update to this PR in ~ a few days; I haven't lost interest :)

@brycemcd
Copy link
Author

Checking in again - I've been waylaid by a bunch of other things that have come up but still am interested in completing this. I haven't given up but I haven't had the time to commit to it yet.

@dwbutler
Copy link
Owner

@brycemcd No worries. What's left, aside from cleanup and my code review feedback? I can make the changes I requested, if that helps.

@brycemcd
Copy link
Author

That's all that's left. It's easy - I just haven't been able to carve out the time. I'm happy to do it as long as there aren't time constraints from your side.

@Saf1u
Copy link

Saf1u commented Nov 9, 2023

@brycemcd @dwbutler sorry to tag out of the blue, but is there a reason this was never merged ? Thank you .

@dwbutler
Copy link
Owner

dwbutler commented Nov 9, 2023

It's because I don't maintain this library anymore. Happy to help transfer it to new owners but nobody has stepped up.

@Saf1u
Copy link

Saf1u commented Nov 9, 2023

Thanks for the reply. I am New to ruby and not experienced with maintaining if not I'd have taken up the offer. Will fork and tinker from there. Thanks and good luck 👍🏽

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

Successfully merging this pull request may close these issues.

4 participants