search

logstash-plugins / logstash-output-google_cloud_storage

Apache License 2.0
9 stars 26 forks source link

Migrated to GCP Java client libraries #21

Closed josephlewis42 closed 5 years ago

josephlewis42 commented 6 years ago

The Ruby GCP client isn't getting a lot of attention and JRuby stopped being supported by the auth library it depends on quite a while back so upgrading is going to be painful if not impossible.

This is a first stab at using the current GCS Java library. It does introduce a breaking change: keys must be provided through ADC or in a JSON key. PKCS keys are a deprecated in GCP.

This may help fix #20

josephlewis42 commented 6 years ago

logstash-input-kafka is having the same issues with their JAR vendoring: https://github.com/logstash-plugins/logstash-input-kafka/issues/241

colinsurprenant commented 6 years ago

the tests failing on 5.6 is not new and not related to this PR - I will bump the priority on this to get to the bottom and solve that.

josephlewis42 commented 6 years ago

@colinsurprenant now that the shutdown handling has been tested in #20, do you think we're good to go?

josephlewis42 commented 6 years ago

@colinsurprenant Alright, I got this moved over to gradle so it now builds for the 5.x branch and updated the documentation around the breaking changes.

I changed two fields from obsolete to deprecated so people using Application Default Credentials won't run into any hiccups when upgrading. The people with the PKCS12 style keys will still need to change them to the JSON style ones, but I've supplied them with a documented way to upgrade those keys.

colinsurprenant commented 6 years ago

Sorry for the delay, got back from vacations. Does this need to be rebased to move it forward?

josephlewis42 commented 6 years ago

@colinsurprenant No worries, hope you had a good time!

I think this will need to be rebased. I'd like to get https://github.com/logstash-plugins/logstash-output-google_cloud_storage/pull/29 merged in first if possible because it's a minor version change.

The Java changes are blocked by a https://github.com/elastic/logstash/issues/9592 which I think is going to get fixed in Logstash 6.3.1 when that gets released so we might have to wait until then.

colinsurprenant commented 6 years ago

@josephlewis42 I added a few minor comments.

One more important issue I see has to do with the jars dependencies. As it is in this PR, all dependencies are declared as compile , for example with jackson in build.grade:

compile "com.fasterxml.jackson.core:jackson-core:2.1.3"

which result in having in lib/logstash-output-google_cloud_storage_jars.rb :

require_jar('com.fasterxml.jackson.core', 'jackson-core', '2.1.3')

So, for jars that are already loaded by logstash core such as jackson, this will likely result in version incompatibilities.

We currently don't have a good story about managing jar dependencies and this will all change once we land the Java API but for now we have to be cautious. I think the best thing to do at this point in our hybrid Ruby/Java model is to declare libraries that are already loaded by logstash core as compileOnly in gradle dependencies. A good example for this would be logstash-input-dead_letter_queue.

Also I see there are still a lot of dependencies. Are they all needed? for example: guava-jdk5 ?

josephlewis42 commented 6 years ago

Hi Colin,

I dropped the ball on updating this one. Thank you for the feedback, for the dependency question I might be doing something incorrectly, here's what I did to gather them:

I ran ./gradlew dependencies and took out the runtime dependency tree and ran that through: sed "s/.*---\(.*\)/\1/" | grep -v "(*)" | sort | uniq | sed "s/\(.*\)/ compile \"\1\"/" and manually resolved any of those dependencies e.g. com.google.protobuf:protobuf-java:3.5.1 -> 3.6.0, com.google.protobuf:protobuf-java:3.6.0 just becomes the latter com.google.protobuf:protobuf-java:3.6.0.

If that looks correct, I'll go ahead and fix the dependencies Logstash already bundles to be compileOnly.

One of the benefits/drawbacks of our client libraries is that they're all auto-generated so they're always up to date but sometimes at the expense of elegance. When upgrading the library version for this update, it looks like that guava-jdk5 was fixed in one of the upstream libs, but the majority are still there.

Thanks!

colinsurprenant commented 5 years ago

@josephlewis42 I left a few code comments, mostly minor stuff, once these are addressed we should be good to merge.

Maybe @karenzone could take a look at the docs, if there are changes it could be deferred into a separate issue.

josephlewis42 commented 5 years ago

Hi @colinsurprenant,

I fixed those things you pointed out and ran tests with 6.6.2 and 7.0.0-beta1. The only thing I had to change for those was dropping the the dependency constraint on ruby-concurrent because the plugin was picking up the -java one which conflicted with Logstash and blocked plugin installation.

josephlewis42 commented 5 years ago

Cool deal, @colinsurprenant would you mind if I merge/tag and released this as 4.0.0 then re-based #34 on top as an NBC as 4.1?

colinsurprenant commented 5 years ago

@josephlewis42 yes absolutely, let's release this as 4.0. Let me know if you need help with that. Then we can finish #34 and do a 4.1 with it.

josephlewis42 commented 5 years ago

Alright, this has been released to rubygems and I've created a tag in this repository for v4.0.0.

@karenzone when you get a chance will you update the docs? If they're not showing up they might still be affected by https://github.com/elastic/logstash/issues/9511

Thank you both!