Update KPL to 0.12.3

[xose]

This PR updates the KPL to version 0.12.1

[sonya]

I wish the author of this PR provided more commentary, as this is potentially a very useful PR.

I did a bit of testing with this branch, as it could address #9 (by removing fixed jars from the repo; you would still be stuck on particular versions once the gem is built).

you have to run rake install_jars prior to building the gem or running rspec, but otherwise the test works. the rake task might need to be added to travis.

I'll try this later with real data/streams.

[sonya]

It looks like these changes work fine with real data. I'm guessing if the Rakefile were updated so that the default tasks are [:vendor, :rspec], that might make travis happy.

If this PR is merged, I would also recommend (at a later stage) updating the main code to use the event.set method rather than setting values directly with []. Doing this would alleviate @samcday's inline comment about empty arrays joining into US-ASCII strings. (Incidentally, SecureRandom.uuid also returns a US-ASCII string; I'm not sure why that's been working).

Anyway @samcday I'd love to know your thoughts on this. I have a few changes I'd like to add that I'd prefer to build on top of this PR rather than libraries without support for event.set.

[samcday]

Hey @sonya thanks for exploring this further.

I'm a little confused by this PR - why did we remove the JARs from the repo? We still end up packing the JARs into the packaged .gem right?

[sonya]

Correct, the jars are loaded at build time by Jars::Installer. Looking at other logstash plugin repos though, it looks like the community is sort of divided about the right way to pull down jar dependencies.

The approach this PR takes is very similar to kafka and geoip. There are PRs on elasticsearch and log4j that take this approach but were ultimately not incorporated. I'm not familiar enough with logstash to really make a good argument either way; here is an interesting a discussion on the logstash repo which is also nonconclusive.

All I can say is that when I build a docker image with my previous configs but build the logstash-output-kinesis gem from this PR instead of installing it from rubygems.org (after running the :vendor step), it works without complaint.

@xose, do you have any comments on your reasoning for taking this approach with jar dependencies?

[xose]

Hey there! First of all sorry for the state of the PR. I have no experience with JRuby and the build system, so I started making changes without foresight which resulted in that commit. I'll try to split it into several ones this weekend.

Regarding the JARs, the main reason to remove them is that the Kinesis Producer jar is huge (>20 MB). So I though it would be better to get all of them out of the repo, or it would grow too big. The repo is already 60 MB when it could be just a few kilobytes if it never had any jars, and each update to the dependencies would make it grow by another 20-25 MB.

I copied the approach from the Kinesis Input plugin, which I believe comes from the Kafka one. If there is a better way to do it, or you want to keep the jars in the repo, please feel free to change it.

[xose]

I have split the commit in several parts and made Travis happy in the process.

[sonya]

The updated changes also look okay to me (though I haven't installed this on a live machine).

The reason I'm interested in this PR is because I've been wanting to make a PR which would support the sprintf format for stream configuration. I have a working diff here based on xose's branch (most of my diff is just adding specs).

I tried writing a version of that change based on master, but that's how I discovered SecureRandom.uuid returns an ascii string. I thought rather than converting strings in the plugin it would be better to use the get and set methods in the new SDK, which work with ascii strings.

(p.s. running bundle exec rake does this slightly annoying thing where it creates untracked files in lib. kafka's plugin does the same thing, while geoip has a line in .gitignore to handle it)

[jbank]

Is there any reason not to merge this pull request?

[sonya]

It looks like this update won't install on Logstash 5 when packaged into a remote gem. Fixing this should be straightforward (it's basically changing the logstash-core requirement into the same versions of logstash-core-plugin-api that other plugins use), but I am going to fork this so I can do more testing with a gem server and stuff

[chrono]

What's the decision about vendoring jars? I have some changes similar to this including Logstash 5 support. I also ended up using the jars installer for that. However, this was only tested locally.

Is this pr here still blocked on this?

[samcday]

logstash-output-kinesis v5.0.0 is live with support for LS 5.x and using KPL 0.12.5 :)