Improved semantics for OutputDelegator #4391

andrewvc · 2015-12-22T22:10:33Z

This commit greatly improves the semantics for the OutputDelegator. It
now:

Has an API for declaring if an output is threadsafe which lets us economize on the number of instances (we only need one shared across N threads)
By default sets the number of workers for an output to be == the
number of pipeline workers (for non-threadsafe outputs)
Moves the worker_safe API (which declares whether the plugin supports > 1 workers) to the class, not instance level
Strongly enforces the worker safety API. Previously a warning would be logged but nothing else.
Should Provide better performance for threadsafe plugins. A plugin like the file output should only need a mutex around the file write and could be written to be threadsafe. This opens the door to multiple threads running, say, their codecs simultaneously.

This commit is built on top of some of the ideas in #4367

This commit greatly improves the semantics for the OutputDelegator. It now: * Has an API for declaring if an output is threadsafe to avoid instance creation * By default sets the number of workers for an output to be == the number of pipeline workers * Moves the worker safe API to the class, not instance level * Strongly enforces the worker safety API * Should provide better performance for threadsafe plugins

suyograo · 2016-01-03T23:26:08Z

@andrewvc this can be for post 2.2, right?

andrewvc · 2016-01-04T00:29:01Z

Yes, it can be! I'd love to get this in for 2.3 however.

ph · 2016-01-04T17:21:05Z

Reviewing today.

andrewvc · 2016-01-04T19:25:54Z

Thanks @ph!

ph · 2016-01-05T15:55:51Z

logstash-core/lib/logstash/output_delegator.rb

+    # The user has configured extra workers, but this plugin doesn't support it :(
+    if @config["workers"] && @config["workers"] > 1 && @klass.workers_not_supported?
+      message = @workers_not_supported_message
+      if message


Do we really need to create a new variable message here we could use @workers_not_supported_message directly?

This line is actually erroneous, it should be pulled from @klass. Will be updated in my next push

ph · 2016-01-05T16:09:49Z

Added minors comments, I haven't tested with actual plugins yet.

But before we run real world config, can we add a few tests to showcase what happen with the different workers settings and the different options set from the plugin? I am looking at the interaction between threadsafe_multi_receive and worker_multi_receive and the actual plugin. I think we could benefit by having them.

ph · 2016-01-05T16:12:32Z

logstash-core/lib/logstash/pipeline.rb

    else
      klass.new(*args)
    end
  end

+  def default_output_workers
+    @settings[:pipeline_workers] || @settings[:default_pipeline_workers]
+  end


Do we really need to have a check for this? The DEFAULT_SETTINGS already have a default settings configured?
I think the fallback to defaults should be done in one place https://github.com/elastic/logstash/pull/4391/files#diff-bc1e9ea81caa54ea0d56abf4415fdf00R34 ? WDYT?

This I think is separate, because default_output_workers is semantically different from default_pipeline_workers, yet it must move in lock-step with the default value of the pipeline workers. Are you sure you aren't confusing the two? IMHO this is the correct place for this.

I think you are right, I was effectively confusing the two, move along!

andrewvc · 2016-01-05T23:00:27Z

@ph I've removed the WIP designation and have added some real tests. How's it look now?

@suyograo would you be interested in being a second reviewer?

Backwards compatibility is now implemented for existing workers_not_supported uses. This clears up a few bugs in the initial pass as well.

suyograo · 2016-01-05T23:09:57Z

I volunteer @jsvd as the second reviewer. I don't understand the internals to this extent :)

ph · 2016-01-06T01:23:25Z

reviewing :)

ph · 2016-01-06T01:24:09Z

logstash-core/lib/logstash/output_delegator.rb

+    #
+    # Older plugins invoke the instance method Outputs::Base#workers_not_supported
+    # To detect these we need an instance to be created first :()
+    # TODO: In the next major version after 2.x remove support for this


Yay for backward compatibility

ph · 2016-01-06T01:32:14Z

@andrewvc Much improvements! we are coming a long way! sir.

ph · 2016-01-06T01:36:28Z

@andrewvc Looking good for me, I'll do a bit of manual testing tomorrow morning.

…LS > 2.2 API changes Fixes #4391

We were getting some bizzare errors (#4391 (comment)) on the file output with the new OutputDelegator changes. By switching away from method(:mymethod) to blocks with define_singleton_method they went away. This bug is only present with Jruby 1.7.x (tested with 1.7.22 and 1.7.23). Jruby 9.x is fine. Fixes #4391

… delegator Fixes #4391

Fixes #4391

…he semantics for the OutputDelegator. It now: * Has an API for declaring if an output is threadsafe to avoid instance creation * By default sets the number of workers for an output to be == the number of pipeline workers * Moves the worker safe API to the class, not instance level * Strongly enforces the worker safety API * Should provide better performance for threadsafe plugins Fixes #4391

Backwards compatibility is now implemented for existing workers_not_supported uses. This clears up a few bugs in the initial pass as well. Fixes #4391

…LS > 2.2 API changes Fixes #4391

We were getting some bizzare errors (#4391 (comment)) on the file output with the new OutputDelegator changes. By switching away from method(:mymethod) to blocks with define_singleton_method they went away. This bug is only present with Jruby 1.7.x (tested with 1.7.22 and 1.7.23). Jruby 9.x is fine. Fixes #4391

… delegator Fixes #4391

Fixes #4391

Backwards compatibility is now implemented for existing workers_not_supported uses. This clears up a few bugs in the initial pass as well. Fixes #4391

…LS > 2.2 API changes Fixes #4391

We were getting some bizzare errors (#4391 (comment)) on the file output with the new OutputDelegator changes. By switching away from method(:mymethod) to blocks with define_singleton_method they went away. This bug is only present with Jruby 1.7.x (tested with 1.7.22 and 1.7.23). Jruby 9.x is fine. Fixes #4391

… delegator Fixes #4391

Fixes #4391

andrewvc added enhancement discuss labels Dec 22, 2015

andrewvc self-assigned this Dec 22, 2015

suyograo added the v2.2.0 label Dec 23, 2015

suyograo added v2.3.0 and removed v2.2.0 labels Jan 4, 2016

ph reviewed Jan 5, 2016
View reviewed changes

andrewvc changed the title ~~[WIP] Improved semantics for OutputDelegator~~ Improved semantics for OutputDelegator Jan 5, 2016

andrewvc force-pushed the better-enforced-output-delegator branch from 6ee6427 to 3900785 Compare January 5, 2016 22:59

Improve OutputDelegator implementation and specs.

9ab024d

Backwards compatibility is now implemented for existing workers_not_supported uses. This clears up a few bugs in the initial pass as well.

andrewvc force-pushed the better-enforced-output-delegator branch from 3900785 to 9ab024d Compare January 5, 2016 23:06

ph reviewed Jan 6, 2016
View reviewed changes

andrewvc mentioned this pull request Jan 6, 2016

Add section to 'How to Write an Output Plugin' covering changes in OutputDelegator #4423

Closed

andrewvc added a commit that referenced this pull request Jan 7, 2016

Update how to create an output plugin docs to include information on …

98e0e35

…LS > 2.2 API changes Fixes #4391

andrewvc added a commit that referenced this pull request Jan 7, 2016

Add debug log for determining the number of output workers for output…

edd1dd1

… delegator Fixes #4391

andrewvc added a commit that referenced this pull request Jan 7, 2016

Cleanup code around warnings around outputs with no workers allowed

2fc233e

Fixes #4391

andrewvc added a commit that referenced this pull request Jan 7, 2016

Remove unnecessary logger line used for debugging in outputdelegator

3b3d507

Fixes #4391

andrewvc added a commit that referenced this pull request Jan 7, 2016

Link to jruby issue in OutputDelegator

aa9f1ca

Fixes #4391

andrewvc added a commit that referenced this pull request Jan 7, 2016

Code cleanup per PR review in OutputDelegator

a8d622b

Fixes #4391

andrewvc added a commit that referenced this pull request Jan 7, 2016

Improve OutputDelegator implementation and specs.

00c74d8

Backwards compatibility is now implemented for existing workers_not_supported uses. This clears up a few bugs in the initial pass as well. Fixes #4391

andrewvc added a commit that referenced this pull request Jan 7, 2016

Update how to create an output plugin docs to include information on …

3f7c6ba

…LS > 2.2 API changes Fixes #4391

andrewvc added a commit that referenced this pull request Jan 7, 2016

Add debug log for determining the number of output workers for output…

d93b4d6

… delegator Fixes #4391

andrewvc added a commit that referenced this pull request Jan 7, 2016

Cleanup code around warnings around outputs with no workers allowed

0770d02

Fixes #4391

andrewvc added a commit that referenced this pull request Jan 7, 2016

Remove unnecessary logger line used for debugging in outputdelegator

379dfd8

Fixes #4391

andrewvc added a commit that referenced this pull request Jan 7, 2016

Link to jruby issue in OutputDelegator

cddf53e

Fixes #4391

andrewvc added a commit that referenced this pull request Jan 7, 2016

Code cleanup per PR review in OutputDelegator

7908931

Fixes #4391

andrewvc closed this in 57f8b65 Jan 7, 2016

andrewvc added a commit that referenced this pull request Jan 7, 2016

Improve OutputDelegator implementation and specs.

f6ff16e

Backwards compatibility is now implemented for existing workers_not_supported uses. This clears up a few bugs in the initial pass as well. Fixes #4391

andrewvc added a commit that referenced this pull request Jan 7, 2016

Update how to create an output plugin docs to include information on …

12d1de0

…LS > 2.2 API changes Fixes #4391

andrewvc added a commit that referenced this pull request Jan 7, 2016

Add debug log for determining the number of output workers for output…

e492ff3

… delegator Fixes #4391

andrewvc added a commit that referenced this pull request Jan 7, 2016

Cleanup code around warnings around outputs with no workers allowed

ad48e00

Fixes #4391

andrewvc added a commit that referenced this pull request Jan 7, 2016

Remove unnecessary logger line used for debugging in outputdelegator

2d477b6

Fixes #4391

andrewvc added a commit that referenced this pull request Jan 7, 2016

Link to jruby issue in OutputDelegator

4e5d2c8

Fixes #4391

andrewvc added a commit that referenced this pull request Jan 7, 2016

Code cleanup per PR review in OutputDelegator

26ba7da

Fixes #4391

suyograo mentioned this pull request Jan 11, 2016

Sane defaults / output worker setup for Logstash 2.2 ng_pipeline #4367

Closed

suyograo added v2.2.0 and removed v2.3.0 labels Jan 12, 2016

ppf2 mentioned this pull request Feb 16, 2016

Parameters no longer support string representation for numeric values in LS 2.2+ #4692

Open

suyograo mentioned this pull request Mar 23, 2016

Revert default setting for # of output workers #4877

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved semantics for OutputDelegator #4391

Improved semantics for OutputDelegator #4391

andrewvc commented Dec 22, 2015

suyograo commented Jan 3, 2016

andrewvc commented Jan 4, 2016

ph commented Jan 4, 2016

andrewvc commented Jan 4, 2016

ph Jan 5, 2016

andrewvc Jan 5, 2016

ph commented Jan 5, 2016

ph Jan 5, 2016

andrewvc Jan 5, 2016

ph Jan 5, 2016

andrewvc commented Jan 5, 2016

suyograo commented Jan 5, 2016

ph commented Jan 6, 2016

ph Jan 6, 2016

ph commented Jan 6, 2016

ph commented Jan 6, 2016

Improved semantics for OutputDelegator #4391

Improved semantics for OutputDelegator #4391

Conversation

andrewvc commented Dec 22, 2015

suyograo commented Jan 3, 2016

andrewvc commented Jan 4, 2016

ph commented Jan 4, 2016

andrewvc commented Jan 4, 2016

ph Jan 5, 2016

Choose a reason for hiding this comment

andrewvc Jan 5, 2016

Choose a reason for hiding this comment

ph commented Jan 5, 2016

ph Jan 5, 2016

Choose a reason for hiding this comment

andrewvc Jan 5, 2016

Choose a reason for hiding this comment

ph Jan 5, 2016

Choose a reason for hiding this comment

andrewvc commented Jan 5, 2016

suyograo commented Jan 5, 2016

ph commented Jan 6, 2016

ph Jan 6, 2016

Choose a reason for hiding this comment

ph commented Jan 6, 2016

ph commented Jan 6, 2016