search

logstash-plugins / logstash-output-sqs

Apache License 2.0
4 stars 22 forks source link

Major rewrite for new concurrency model #20

Closed joshuaspence closed 7 years ago

joshuaspence commented 7 years ago

This is a fairly major rewrite of the plugin, including the following changes:

jrgns commented 7 years ago

@joshuaspence Thanx for this, it's a big step in the right direction.

There are a couple of issues, though, which will prevent me from merging this in. Can I ask you to create a new PR with only the following?

The problem lies with the changes to the changelog and the removal of Notice.txt. If there are more style issues you'd like to fix (like cleaning up the gemspec) you're welcome to submit a separate PR for that.

Edit: Oh, and I'll try and test this branch in the mean time!

Thanx!

joshuaspence commented 7 years ago

Yep, can do. I got a bit carried away with tidying things up.

jrgns commented 7 years ago

@joshuaspence While you're busy, please add a version to the example policy, and remove the empty Sid. Ideally a user should be able to copy and paste the policy with the minimum of modifications.

joshuaspence commented 7 years ago

By the way, do you know why Travis is failing? I've had this failing locally as well, and I'm able to work around it by installing a non-snapshot version of logstash-core.

NameError: missing class name (`org.apache.logging.log4j.Level')
  org/jruby/javasupport/JavaUtilities.java:54:in `get_proxy_or_package_under_package'
  file:/home/travis/.rvm/rubies/jruby-1.7.25/lib/jruby.jar!/jruby/java/java_package_module_template.rb:14:in `method_missing'
  /home/travis/build/logstash-plugins/logstash-output-sqs/vendor/bundle/jruby/1.9/gems/logstash-core-5.1.2.snapshot1-java/lib/logstash/logging/logger.rb:6:in `Logging'
  /home/travis/build/logstash-plugins/logstash-output-sqs/vendor/bundle/jruby/1.9/gems/logstash-core-5.1.2.snapshot1-java/lib/logstash/logging/logger.rb:5:in `LogStash'
  /home/travis/build/logstash-plugins/logstash-output-sqs/vendor/bundle/jruby/1.9/gems/logstash-core-5.1.2.snapshot1-java/lib/logstash/logging/logger.rb:4:in `(root)'
  org/jruby/RubyKernel.java:1040:in `require'
  /home/travis/build/logstash-plugins/logstash-output-sqs/vendor/bundle/jruby/1.9/gems/logstash-core-5.1.2.snapshot1-java/lib/logstash/util/loggable.rb:1:in `(root)'
  org/jruby/RubyKernel.java:1040:in `require'
  /home/travis/build/logstash-plugins/logstash-output-sqs/vendor/bundle/jruby/1.9/gems/logstash-core-5.1.2.snapshot1-java/lib/logstash/util/loggable.rb:2:in `(root)'
  org/jruby/RubyKernel.java:1040:in `require'
  /home/travis/build/logstash-plugins/logstash-output-sqs/vendor/bundle/jruby/1.9/gems/logstash-core-5.1.2.snapshot1-java/lib/logstash/settings.rb:1:in `(root)'
  org/jruby/RubyKernel.java:1040:in `require'
  /home/travis/build/logstash-plugins/logstash-output-sqs/vendor/bundle/jruby/1.9/gems/logstash-core-5.1.2.snapshot1-java/lib/logstash/settings.rb:2:in `(root)'
  org/jruby/RubyKernel.java:1040:in `require'
  /home/travis/build/logstash-plugins/logstash-output-sqs/vendor/bundle/jruby/1.9/gems/logstash-core-5.1.2.snapshot1-java/lib/logstash/environment.rb:1:in `(root)'
  org/jruby/RubyKernel.java:1040:in `require'
  /home/travis/build/logstash-plugins/logstash-output-sqs/vendor/bundle/jruby/1.9/gems/logstash-core-5.1.2.snapshot1-java/lib/logstash/environment.rb:5:in `(root)'
  org/jruby/RubyKernel.java:1059:in `load'
  /home/travis/build/logstash-plugins/logstash-output-sqs/vendor/bundle/jruby/1.9/gems/logstash-core-5.1.2.snapshot1-java/lib/logstash/json.rb:1:in `(root)'
  org/jruby/RubyArray.java:1613:in `each'
  /home/travis/build/logstash-plugins/logstash-output-sqs/vendor/bundle/jruby/1.9/gems/logstash-core-5.1.2.snapshot1-java/lib/logstash/json.rb:2:in `(root)'
  /home/travis/build/logstash-plugins/logstash-output-sqs/vendor/bundle/jruby/1.9/gems/logstash-core-event-java-5.1.2.snapshot1-java/lib/logstash/event.rb:1:in `(root)'
  /home/travis/build/logstash-plugins/logstash-output-sqs/vendor/bundle/jruby/1.9/gems/logstash-core-event-java-5.1.2.snapshot1-java/lib/logstash/event.rb:4:in `(root)'
  /home/travis/build/logstash-plugins/logstash-output-sqs/spec/integration/outputs/sqs_spec.rb:1:in `(root)'
  /home/travis/build/logstash-plugins/logstash-output-sqs/spec/integration/outputs/sqs_spec.rb:3:in `(root)'
  /home/travis/build/logstash-plugins/logstash-output-sqs/vendor/bundle/jruby/1.9/gems/rspec-core-3.5.4/lib/rspec/core/configuration.rb:1:in `(root)'
  org/jruby/RubyKernel.java:1059:in `load'
  /home/travis/build/logstash-plugins/logstash-output-sqs/vendor/bundle/jruby/1.9/gems/rspec-core-3.5.4/lib/rspec/core/configuration.rb:1435:in `load_spec_files'
  org/jruby/RubyKernel.java:1059:in `load'
  /home/travis/build/logstash-plugins/logstash-output-sqs/vendor/bundle/jruby/1.9/gems/rspec-core-3.5.4/lib/rspec/core/configuration.rb:1433:in `load_spec_files'
  /home/travis/build/logstash-plugins/logstash-output-sqs/vendor/bundle/jruby/1.9/gems/rspec-core-3.5.4/lib/rspec/core/runner.rb:100:in `setup'
  /home/travis/build/logstash-plugins/logstash-output-sqs/vendor/bundle/jruby/1.9/gems/rspec-core-3.5.4/lib/rspec/core/runner.rb:86:in `run'
  /home/travis/build/logstash-plugins/logstash-output-sqs/vendor/bundle/jruby/1.9/gems/rspec-core-3.5.4/lib/rspec/core/runner.rb:71:in `run'
  /home/travis/build/logstash-plugins/logstash-output-sqs/vendor/bundle/jruby/1.9/gems/rspec-core-3.5.4/lib/rspec/core/runner.rb:45:in `invoke'
  /home/travis/build/logstash-plugins/logstash-output-sqs/vendor/bundle/jruby/1.9/gems/rspec-core-3.5.4/exe/rspec:4:in `(root)'
  /home/travis/build/logstash-plugins/logstash-output-sqs/vendor/bundle/jruby/1.9/bin/rspec:1:in `(root)'
  /home/travis/build/logstash-plugins/logstash-output-sqs/vendor/bundle/jruby/1.9/bin/rspec:23:in `(root)'
ph commented 7 years ago

@joshuaspence <3 the changes you have made, I will try to complete review of the code asap since its similar to the s3 changes that just got merged.

Since we move to `#multi_receive_encoded, remove the stud:buffer and we have added a bunch of unit test, I would probably do a major version bump

@jrgns if you test can test it in real live can you report your observation here?

The log4j classes issue I thought we have fixed that in a snapshot releases.

ph commented 7 years ago

@joshuaspence Concerning the log4j bug, in your spec make sure to have the following line before anything else, we have have made some changes for the persistent queue that require this.

require "logstash/devutils/rspec/spec_helper"

Also make sure you have the latest devutils, it probably better to remove the *.lock in your project to make bundler regenerate the whole dependency tree.

jrgns commented 7 years ago

@ph Thanx for jumping in :) I have to get a test env up and running again, but should be able to test and report back later today.

joshuaspence commented 7 years ago

@ph, sure I will bump the version to 4.0.0.

joshuaspence commented 7 years ago

Hmm, I'm having trouble with dependencies and I don't think that it's my fault...

Could not find gem 'logstash (< 2.0.0, >= 1.4.0) java', which is required by gem
'logstash-input-generator java', in any of the sources.Bundler could not find
compatible versions for gem "logstash-core":
  In Gemfile:
    logstash-output-sqs java was resolved to 3.1.0, which depends on
logstash-core-plugin-api (<= 2.99, >= 1.60) java was resolved to 1.60.0,
which depends on
        logstash-core (<= 2.4.99, >= 2.4.0.snapshot1) java

    logstash-input-generator java was resolved to 0.1.3, which depends on
      logstash-core (< 2.0.0, >= 1.4.0) java

    logstash-input-generator java was resolved to 0.1.3, which depends on
      logstash-core (< 2.0.0, >= 1.4.0) java

    logstash-input-generator java was resolved to 0.1.3, which depends on
      logstash-core (< 2.0.0, >= 1.4.0) java
ph commented 7 years ago

@joshuaspence Are you running under jruby?

joshuaspence commented 7 years ago

Yep.

> ruby --version
jruby 1.7.25 (1.9.3p551) 2016-04-13 867cb81 on Java HotSpot(TM) 64-Bit Server VM 1.8.0_111-b14 +jit [linux-amd64]
joshuaspence commented 7 years ago

It seems that the problem is the runtime dependency on aws-sdk. If I remove that dependency, bundle install works fine. Is it considered good practice to add an explicit dependency on the aws-sdk gem? It doesn't seem to be strictly necessary.

joshuaspence commented 7 years ago

@ph, thanks! The issue with log4j was the ordering of my requires.

ph commented 7 years ago

@joshuaspence I was able to reproduce your issue and removing the aws-sdk solved the issue, but you are right we should explicitely depend on aws-sdk this dependency should come from the mixin.

ping us went is ready for a round a review.

joshuaspence commented 7 years ago

you are right we should explicitely depend on aws-sdk this dependency should come from the mixin.

This seems contradictory... Should the dependency on aws-sdk be explicit, or is the dependency on logstash-mixin-aws sufficient?

joshuaspence commented 7 years ago

This is ready for review, although I want to add some more test coverage.

ph commented 7 years ago

You are right we should not explicitely depends on aws-sdk.

my bad :)

joshuaspence commented 7 years ago

I plan to also add some more documentation, but feel free to review in the mean time. Any feedback would be appreciated as I am not a Ruby developer and this is my first time writing RSpec tests.

jrgns commented 7 years ago

Nice, functional tests on this side went well :)

jrgns commented 7 years ago

This will fix issues #5 #7 and #11

ph commented 7 years ago

@joshuaspence I think we are pretty close to get this merged, can you address the few issue from the review?

joshuaspence commented 7 years ago

I'll push my latest changes now. I haven't yet addressed any of the most recent feedback, but I have expanded the integration tests slightly to upload more than a single message.

joshuaspence commented 7 years ago

I have a general question here, again because I'm not really a Ruby developer. Whilst working on this PR, I read quite a bit of Logstash code (mostly plugins) and I'm not sure what the preferred way to require the spec_helper.rb file is. It seems that there are two methods for doing so:

Is there any preference here?

ph commented 7 years ago

@joshuaspence These two methods have different behavior.

require 'spec_helper'

This load the first file it match in the global ruby $LOAD_PATH,

require_relative '../../spec_helper'

Ruby will expand the relative path from the current file and load it.

So if you want to make sure it load your file in the plugin repo in all execution context you must use the second form.

joshuaspence commented 7 years ago

@ph, thanks for the explanation of require_relative

joshuaspence commented 7 years ago

Hmm, if require-ing dependencies explicitly is preferred, I feel that we should add a runtime dependency on aws-sdk.

ph commented 7 years ago

@joshuaspence Most of our code require the ruby file when they need it; this way the code is only loaded into the program namespace when a plugin is defined in the logstash config, reducing the memory at the same time.

We don't need an explicit runtime dependency on the aws-sdk, since we assume that another gem will bring it. In your case the dependency will be coming from the logstash-mixin-aws gem this give us more flexibility on versioning since multiple plugins use the AWS-SDK, some v1 and some v2, it just easier to change it in a single place.

joshuaspence commented 7 years ago

I've addressed most of your feedback. Still a bit more work to do though.

joshuaspence commented 7 years ago

Most of our code require the ruby file when they need it; this way the code is only loaded into the program namespace when a plugin is defined in the logstash config, reducing the memory at the same time.

Does this mean that I should move the require 'aws-sdk' to #register?

ph commented 7 years ago

Does this mean that I should move the require 'aws-sdk'

No you achieve the same lazy load when you define it at the top of the plugin file and this is easier to reason with in your tests.

In some older plugins you will see some require in the register, we remove them as we see them.

Sometime they need to be in the register since we do some patching on libraries or hack, but these cases are really rare.

jrgns commented 7 years ago

Looks good to me. @ph Any last comments?

ph commented 7 years ago

@jrgns @joshuaspence I think the only missing thing from this PR is to refactor the code to use the same path when the batch is set to true or false using the batch_events = 1 trick?

Sorry I was christmas mode 🦃 🎄 🎅

joshuaspence commented 7 years ago

I have also been in Christmas mode. Will finish this off in the next few days.

joshuaspence commented 7 years ago

I believe that all of your concerns have been addressed. Let me know if there are any other changes to be made.

joshuaspence commented 7 years ago

OK sure, I think I misunderstood your comments regarding batch_events. Will fix them now.

Sorry about the constant rebasing, it is just what I am used to in my normal workflow.

ph commented 7 years ago

I've released 4.0.0 on rubygems good work @joshuaspence