Replace `def #{type}_func` with define_singleton_method #4298

ph · 2015-12-02T21:46:20Z

When you run multiples pipeline and the config code get evaluated, on
every evaluation the class cached is clear, everytime you were calling a
func method you had the latest evaluated code. The filter_func and
the output_func need to be unique for every instance of the pipeline,
this PR replace the def with a define_single_method call ensuring
the uniqueness of the code on every instance.

This PR is based on #4254

andrewvc · 2015-12-02T22:35:28Z

LGTM, just waiting for #4254 to merge in

purbon · 2015-12-03T08:49:22Z

@andrewvc @ph I think this change is not just for your #4254, right? I can also go to master now, right? If not, can we point this PR to your PR and merge it there? or also point it to the feature branch? then we don't have to wait.

what do you say?

purbon · 2015-12-03T08:51:11Z

@ph having @andrewvc commits in this PR it does really make the review more complicated. hard to spot where are your changes for example. I would recommend to simplify the dependency as I shared in my previous comment.

purbon · 2015-12-03T09:19:07Z

logstash-core/spec/logstash/pipeline_spec.rb

@@ -15,6 +15,21 @@ def close
  end
 end

+class DummyInputGenerator < LogStash::Inputs::Base


we install the generator plugin as part of the core plugins need for test, https://github.com/elastic/logstash/blob/master/rakelib/default_plugins.rb#L113-L121 any reason not to redefined here? you know I would like our make our test plugin free, only use mocks/stubs/dummy (pick your name), but if we require it for now I think it would be better to stick to one usage. what do you think?

I was thinking to use the generator but I have checked the logstash-core gemspec and it doesn't include the generator plugin and the only test in logstash-core that require the generator is the pipeline_reporter_spec.

Its not obvious that the generator is available in the context of theses specs 😞

I know is not obvious, is in the file I shared, 👍 I understand. I think we should have only 1 way if possible, right? what do you think?

Well, lets keep the DummyGenerator, since we don't need a dependency for that and you were proposing to move away. This also in line with the actual dummy plugin in the current pipeline_spec

are we already ok on going away from plugins to be used in specs? I don't think we agreed, even if I love the idea. not sure what to think.

ph · 2015-12-03T14:15:00Z

@purbon Concerning this PR against master vs the branch, let me check If I can do it agains't master.

ph · 2015-12-03T14:44:32Z

I will remake this PR against master

ph · 2015-12-03T14:50:20Z

@purbon It can be done, but the test would change, so lets keep it for #4254

purbon · 2015-12-03T14:52:06Z

@ph then I would recommend to point to @andrewvc branch/PR/whatever. It makes no sense we wait to incorporate this if is good enoughts. makes sense? @andrewvc what do you think?

ph · 2015-12-03T15:24:55Z

updated with the review, @andrewvc do you want to cherry-pick this or I can create the PR against a feature branch?

andrewvc · 2015-12-03T16:51:27Z

@ph well, I don't want to reopen the pr for #4254 so maybe we can wait till that gets in master? I think I can address all of the open concerns today.

PRs against PRs are hard :(

ph · 2015-12-03T16:52:02Z

@andrewvc np I will rebase, when @purbon gives his lgtm.

purbon · 2015-12-03T17:12:30Z

LGTM

andrewvc · 2015-12-31T20:08:25Z

Hey @ph are you planning on rebasing this?

ph · 2016-01-05T14:37:01Z

rebased with master @andrewvc !

andrewvc · 2016-01-05T16:19:04Z

logstash-core/spec/logstash/pipeline_spec.rb

@@ -47,6 +47,10 @@ def initialize(params={})

  def register
  end
+
+  def threadsafe?
+    false


I don't think this currently exists as an API. In https://github.com/elastic/logstash/pull/4391/files#diff-3d93d6656c81049b6de7a09dbe141a46R30 I added a new method Outputs/Base.declare_threadsafe! that would declare this at the class level.

let me check that, if its needed, I think I had an error without it.

You were right, I think at the time I have created the pr it was required.

ph · 2016-01-05T16:29:18Z

updated with your comment @andrewvc

andrewvc · 2016-01-05T16:47:51Z

logstash-core/spec/logstash/pipeline_spec.rb

+    let(:pipeline2) { LogStash::Pipeline.new("input { generator {} } filter { dummyfilter {} } output { dummyoutput {}}") }
+
+    it "should handle evaluating different config" do
+      expect(pipeline1.output_func(LogStash::Event.new)).not_to include(nil)


This could use a comment explaining why we're checking for it not to be nil.

andrewvc · 2016-01-05T16:48:05Z

Could use an explanatory comment, but after that LGTM!

When you run multiples pipeline and the config code get evaluated, on every evaluation the class cached is clear, everytime you were calling a `func` method you had the latest evaluated code. The `filter_func` and the `output_func` need to be unique for every instance of the pipeline, this PR replace the `def` with a `define_single_method` call ensuring the uniqueness of the code on every instance. This PR is based on #4254

ph · 2016-01-05T17:01:49Z

Added a comment!

elasticsearch-bot · 2016-01-06T20:24:54Z

Pier-Hugues Pellerin merged this into the following branches!

Branch	Commits
master	`9d876fa`
2.x	`da26b88`
2.2	`27e675c`

When you run multiples pipeline and the config code get evaluated, on every evaluation the class cached is clear, everytime you were calling a `func` method you had the latest evaluated code. The `filter_func` and the `output_func` need to be unique for every instance of the pipeline, this PR replace the `def` with a `define_single_method` call ensuring the uniqueness of the code on every instance. This PR is based on #4254 Fixes #4298

ph assigned andrewvc Dec 2, 2015

ph added the v2.2.0 label Dec 2, 2015

ph changed the title ~~Fix/allow multiples pipeline config~~ Replace def #{type}_func with define_singleton_method Dec 2, 2015

ph force-pushed the fix/allow-multiples-pipeline-config branch 2 times, most recently from 693f709 to 6cf2950 Compare December 2, 2015 21:55

purbon reviewed Dec 3, 2015
View reviewed changes

ph force-pushed the fix/allow-multiples-pipeline-config branch 2 times, most recently from 9dd0d6f to d4bea94 Compare December 3, 2015 15:24

ph mentioned this pull request Dec 3, 2015

Next Gen Pipeline #4254

Closed

ph force-pushed the fix/allow-multiples-pipeline-config branch 2 times, most recently from 43b012a to b259cdd Compare January 5, 2016 14:29

andrewvc reviewed Jan 5, 2016
View reviewed changes

ph force-pushed the fix/allow-multiples-pipeline-config branch from b259cdd to 4d83693 Compare January 5, 2016 16:27

andrewvc reviewed Jan 5, 2016
View reviewed changes

ph force-pushed the fix/allow-multiples-pipeline-config branch from 4d83693 to c032468 Compare January 5, 2016 17:01

ph closed this in 9d876fa Jan 6, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `def #{type}_func` with define_singleton_method #4298

Replace `def #{type}_func` with define_singleton_method #4298

ph commented Dec 2, 2015

andrewvc commented Dec 2, 2015

purbon commented Dec 3, 2015

purbon commented Dec 3, 2015

purbon Dec 3, 2015

ph Dec 3, 2015

purbon Dec 3, 2015

ph Dec 3, 2015

purbon Dec 3, 2015

ph commented Dec 3, 2015

ph commented Dec 3, 2015

ph commented Dec 3, 2015

purbon commented Dec 3, 2015

ph commented Dec 3, 2015

andrewvc commented Dec 3, 2015

ph commented Dec 3, 2015

purbon commented Dec 3, 2015

andrewvc commented Dec 31, 2015

ph commented Jan 5, 2016

andrewvc Jan 5, 2016

ph Jan 5, 2016

ph Jan 5, 2016

ph commented Jan 5, 2016

andrewvc Jan 5, 2016

andrewvc commented Jan 5, 2016

ph commented Jan 5, 2016

elasticsearch-bot commented Jan 6, 2016

Replace def #{type}_func with define_singleton_method #4298

Replace def #{type}_func with define_singleton_method #4298

Conversation

ph commented Dec 2, 2015

andrewvc commented Dec 2, 2015

purbon commented Dec 3, 2015

purbon commented Dec 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ph commented Dec 3, 2015

ph commented Dec 3, 2015

ph commented Dec 3, 2015

purbon commented Dec 3, 2015

ph commented Dec 3, 2015

andrewvc commented Dec 3, 2015

ph commented Dec 3, 2015

purbon commented Dec 3, 2015

andrewvc commented Dec 31, 2015

ph commented Jan 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ph commented Jan 5, 2016

Choose a reason for hiding this comment

andrewvc commented Jan 5, 2016

ph commented Jan 5, 2016

elasticsearch-bot commented Jan 6, 2016

Replace `def #{type}_func` with define_singleton_method #4298

Replace `def #{type}_func` with define_singleton_method #4298