Break up large bulk requests into multiple requests. #497

andrewvc · 2016-10-26T19:57:01Z

This PR changes the HTTP Client to try as best as it can to restrict bulk requests to 20MB or less. If a single document is larger than this it will be sent alone in a separate request.

This PR also changes the flush_size to be effectively infinite. If users have explicitly set the flush_size it will still work until that option is made obsolete.

Fixes #164, #284, #382, #453, #499

andrewvc · 2016-10-26T20:00:09Z

I'm thinking 75MB may be too high, but I set it high because if we set it too low we'll reject legit documents that are large. Maybe we need two options max_bulk_megabytes for the maximum request and target_bulk_megabytes for the size to attempt to hit. We would still send if the bulk size winds up being larger than target_bulk_megabytes and only drop / log if the doc is larger than max_bulk_megabytes. Thoughts?

jordansissel · 2016-10-28T17:46:03Z

lib/logstash/outputs/elasticsearch.rb

+
+  # The maximum size an individual document can be without being dropped and logged
+  # Elasticsearch will not accept HTTP requests > 100MB. You likely do not want to increase this size
+  config :max_doc_megabytes, :validate => :number, :default => 90


I feel like we don't' need this setting. I think we can have the code know the max size that will be rejected by Elasticsearch and just use that internally.

Can you tell me a story of why a user would want to set this?

andrewvc#3 moves to use bytes instead of number.

jordansissel · 2016-10-28T17:51:11Z

lib/logstash/outputs/elasticsearch.rb

+  # after which it will reject the request. If the batch received by this plugin exceeds this amount
+  # it will be split up and sent in multiple requests. A request may exceed this size if there is a single
+  # document larger than it. You can limit an individuals document's size with max_doc_megabytes
+  config :target_bulk_megabytes, :validate => :number, :default => 75


I greatly prefer base units for settings, so bytes instead of megabytes.

I'll send you a PR to add bytes to your branch.

suyograo · 2016-10-28T19:14:59Z

lib/logstash/outputs/elasticsearch/http_client.rb

+          next
+        end
+
+        if (bulk_body.size + as_json.size) > @target_bulk_bytes


Do we have to call bulk_body.size every time? Maybe we memoize this size, for the in-flight bulk_body? As in, keep a running total of the current batch size. You only need to calculate size for current doc every time, and then add it to bulk_body_size. Then reset bulk_body_size when the request is complete or when an error happens. WDYT?

This is basically memoized in the internal impl.

jordansissel · 2016-10-28T20:12:13Z

lib/logstash/outputs/elasticsearch/common_configs.rb

-      # save for the case where a filter increases the size of an inflight batch by outputting
-      # events.
-      mod.config :flush_size, :validate => :number, :default => 500
+      # DEPRECATED. We now restrict bulk sizes to a target size of 20MB. Larger events


This might scare users.

Maybe say "We now automatically send the best-sized bulk request where the size is chosen to minimize network latency and memory pressure. Internally, this means we will split a bulk request into multiple requests if they would exceed 20MB in size."

Main goal was to express the intent and also to avoid the word "restrict"

jordansissel · 2016-10-28T20:29:12Z

lib/logstash/outputs/elasticsearch/http_client.rb

+      bulk_body = ""
+      bulk_responses = []
+      bulk_actions.each do |action|
+        as_json = action.is_a?(Array) ?


when would action be an array?

update_action_builder returns either a a single action or an array of them (some bulk things take up two lines, like indexing). Before this was solved via a call to .flatten

jordansissel · 2016-10-28T20:30:01Z

lib/logstash/outputs/elasticsearch/http_client.rb

+    end
+
+    # Flatten array by one level only
+    def flatten_one_array(arr)


missing code?

@jordansissel this is for extra flat arrays... ;)

So flat it's zero-dimensional! Computers are cool!

ph · 2016-10-28T20:34:21Z

lib/logstash/outputs/elasticsearch/http_client.rb

+                    action.map {|line| LogStash::Json.dump(line)}.join("\n") :
+                    LogStash::Json.dump(action)
+
+        if (bulk_body.size + as_json.size) > TARGET_BULK_BYTES


should it be bytesize here?

Good catch. My brain is still in ruby 1.8 where utf-8 strings weren't common. +1 to .bytesize

@jordansissel Well, It help when your language make things breaks :)

jordansissel · 2016-10-28T21:44:53Z

@andrewvc I just scanned a few old issues. Can you make your commits say that they fix #164, #284, #382, #453, #499

jordansissel · 2016-10-28T21:47:56Z

Also elastic/logstash#4000

jsvd · 2016-10-31T11:01:05Z

lib/logstash/outputs/elasticsearch/common_configs.rb

-      # events.
-      mod.config :flush_size, :validate => :number, :default => 500
+      # DEPRECATED. We now restrict bulk sizes to a size that balances heap size with reducing round-trips. Very large events will be sent in a single request by themselves.
+      mod.config :flush_size, :validate => :number, :default => 500, :deprecated => "No Longer Needed"


This PR actually removes the feature (what we can "obsolete"), instead of deprecating it. Since deprecation is out of this "Add max_bulk_megabytes option" PR scope, can we do it in a separate one?

You make a good point, but I think this is still a good use of deprecate. In this case we don't want to break configs, but we want people to stop explicitly setting this. We could leave this feature in, but that's bad because it does things no one should want to do.

I'm open to hearing a conflicting opinion here of course, but there's no good reason to set flush_size once this PR lands IMHO.

I agree with everything but this being a good use of deprecate.
Tagging an option as deprecated and making it a noop can be very dangerous (remember this setting is non default very frequently). I don't feel comfortable in assuming flush_size is useless for everyone and simply swapping the flush heuristic from count to size will be a win for everyone.
Is for this lack of certainty that we have the deprecate -> obsolete cycle, so we can test the waters, provide a fall back for unexpected scenarios and also a smoother migration.

OK, well, I'm fine deprecating it and preserving old behavior for now. But I would like to remove this option before 6.0.

@jsvd my original intent with deprecate was, at the configuration interface, that a configuration would still be loaded successfully but we would warn a user about the deprecation. With obsolete, my intent was to fail to load the config but tell the user why.

From this, I think making it :deprecated as a no-op is fine, since the config still runs.

You have a good point about the folks setting flush_size previously, though, but to be honest, all the uses I can think of are basically workarounds for perceived for actual problems with how much we send to Elasticsearch in a single request. This patch hopefully will remove those problems (perceived or actual) with Elasticsearch bulk sizes, and in doing so, will remove the need for that band-aid -- therefore, I think it's OK to immediately turn :flush_size into a no-op.

Thoughts? Am I missing a key reason that isn't a work-around for why someone would set flush_size?

@jsvd in your favor, the most minimal user-experience change would be to mark it deprecated, set the default to nil, but keep the existing behavior if someone sets flush_size directly.

+1 I'm glad like to see flush_size go away, but pretty much all production cases I've seen tune that setting in some way, which is why I'm concerned with removing it.

jordansissel · 2016-11-04T19:23:01Z

lib/logstash/outputs/elasticsearch/common_configs.rb

@@ -81,7 +81,7 @@ def self.included(mod)
      # If you specify a number larger than the batch size of your pipeline it will have no effect,
      # save for the case where a filter increases the size of an inflight batch by outputting
      # events.
-      mod.config :flush_size, :validate => :number, :default => 500
+      mod.config :flush_size, :validate => :number, :default => 99999999, :deprecate => "This setting is no longer necessary as we now try to restrict bulk requests to sane sizes. See the 'Batch Sizes' section of the docs. If you think you still need to restrict payloads based on the number, not size, of events, please open a ticket."


can we default to nil? A value of 9999999 looks weird (will show up in the docs) and confuses me, and may confuse users.

Yeah, good point!

jordansissel · 2016-11-04T19:24:33Z

lib/logstash/outputs/elasticsearch/http_client.rb

@@ -5,6 +5,8 @@
 require 'logstash/outputs/elasticsearch/http_client/manticore_adapter'

 module LogStash; module Outputs; class ElasticSearch;
+  TARGET_BULK_BYTES = 20 * 1024 * 1024


Put a comment here something maybe explaining why we made this a constant instead of a tunable? My thought is that I want to avoid having users yelling at us about a "bad decision" making this not configurable (even though we agreed to make this not a configuration setting)

Will add a comment!

jordansissel · 2016-11-04T22:14:15Z

lib/logstash/outputs/elasticsearch/common_configs.rb

@@ -81,7 +81,7 @@ def self.included(mod)
      # If you specify a number larger than the batch size of your pipeline it will have no effect,
      # save for the case where a filter increases the size of an inflight batch by outputting
      # events.
-      mod.config :flush_size, :validate => :number, :default => 500
+      mod.config :flush_size, :validate => :number, :deprecate => "This setting is no longer necessary as we now try to restrict bulk requests to sane sizes. See the 'Batch Sizes' section of the docs. If you think you still need to restrict payloads based on the number, not size, of events, please open a ticket."


Typo, :deprecate should be :deprecated

reference: https://github.com/elastic/logstash/blob/master/logstash-core/lib/logstash/config/mixin.rb#L66

jordansissel · 2016-11-04T22:14:55Z

Left one comment. Once this typo is fixed, you have my lgtm! :)

Got his LGTM post typo

elasticsearch-bot · 2016-11-04T22:46:52Z

Andrew Cholakian merged this into the following branches!

Branch	Commits
master	`dc61ac1`, `6d83875`, `68612e6`

Fixes #497

andrewvc added the enhancement label Oct 26, 2016

andrewvc assigned andrewvc and jordansissel Oct 26, 2016

andrewvc added the work in progress label Oct 26, 2016

andrewvc force-pushed the fix-max-bulk-size branch from ec4a02d to 227a369 Compare October 28, 2016 14:51

andrewvc changed the title ~~Add max_bulk_megabytes option~~ [WIP] Add max_bulk_megabytes option Oct 28, 2016

suyograo mentioned this pull request Oct 28, 2016

Add a max event size for pipeline elastic/logstash#6146

Closed

jordansissel reviewed Oct 28, 2016

View reviewed changes

suyograo reviewed Oct 28, 2016

View reviewed changes

andrewvc force-pushed the fix-max-bulk-size branch from 227a369 to a7a05cd Compare October 28, 2016 20:04

jordansissel suggested changes Oct 28, 2016

View reviewed changes

ph reviewed Oct 28, 2016

View reviewed changes

andrewvc force-pushed the fix-max-bulk-size branch from a7a05cd to 852c3fa Compare October 28, 2016 22:25

jsvd reviewed Oct 31, 2016

View reviewed changes

andrewvc force-pushed the fix-max-bulk-size branch 5 times, most recently from 0c0f989 to fcf99f4 Compare November 3, 2016 20:34

andrewvc removed the work in progress label Nov 3, 2016

andrewvc changed the title ~~[WIP] Add max_bulk_megabytes option~~ Add max_bulk_megabytes option Nov 3, 2016

andrewvc force-pushed the fix-max-bulk-size branch from fcf99f4 to fedb210 Compare November 4, 2016 01:41

andrewvc force-pushed the fix-max-bulk-size branch from fedb210 to c06dd4d Compare November 4, 2016 19:19

jordansissel suggested changes Nov 4, 2016

View reviewed changes

jordansissel changed the title ~~Add max_bulk_megabytes option~~ Break up large bulk requests into multiple requests. Nov 4, 2016

Cap bulk requests based on size (20MB)

8363edc

andrewvc force-pushed the fix-max-bulk-size branch from c06dd4d to 8363edc Compare November 4, 2016 19:48

andrewvc added 2 commits November 4, 2016 15:13

Pin addressable gem so as not to require ruby 2.0.0

ac0ad92

Bump to 5.3.0

afdb8b6

jordansissel previously requested changes Nov 4, 2016

View reviewed changes

elasticsearch-bot closed this in dc61ac1 Nov 4, 2016

elasticsearch-bot pushed a commit that referenced this pull request Nov 4, 2016

Pin addressable gem so as not to require ruby 2.0.0

6d83875

Fixes #497

elasticsearch-bot pushed a commit that referenced this pull request Nov 4, 2016

Bump to 5.3.0

68612e6

Fixes #497

acchen97 mentioned this pull request May 23, 2017

Split by bytes filter elastic/logstash#4335

Closed

ph mentioned this pull request Oct 31, 2019

Infinite ingestion retry when batches are too large and using GuaranteedSend elastic/beats#14350

Closed

rdner mentioned this pull request Jan 10, 2022

Split large batches of documents if received 413 from Elasticsearch elastic/beats#29778

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Break up large bulk requests into multiple requests. #497

Break up large bulk requests into multiple requests. #497

andrewvc commented Oct 26, 2016 •

edited

Loading

andrewvc commented Oct 26, 2016

jordansissel Oct 28, 2016

jordansissel Oct 28, 2016

jordansissel Oct 28, 2016

suyograo Oct 28, 2016 •

edited

Loading

andrewvc Oct 28, 2016

jordansissel Oct 28, 2016

jordansissel Oct 28, 2016

andrewvc Nov 3, 2016

jordansissel Oct 28, 2016

andrewvc Oct 28, 2016

jordansissel Oct 28, 2016

ph Oct 28, 2016

jordansissel Oct 28, 2016

ph Oct 28, 2016

jordansissel commented Oct 28, 2016

jordansissel commented Oct 28, 2016

jsvd Oct 31, 2016

andrewvc Oct 31, 2016

jsvd Oct 31, 2016

andrewvc Oct 31, 2016

jordansissel Nov 2, 2016

jordansissel Nov 2, 2016

jsvd Nov 2, 2016

jordansissel Nov 4, 2016

andrewvc Nov 4, 2016

jordansissel Nov 4, 2016

andrewvc Nov 4, 2016

jordansissel Nov 4, 2016

jordansissel commented Nov 4, 2016

elasticsearch-bot commented Nov 4, 2016

Break up large bulk requests into multiple requests. #497

Break up large bulk requests into multiple requests. #497

Conversation

andrewvc commented Oct 26, 2016 • edited Loading

andrewvc commented Oct 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suyograo Oct 28, 2016 • edited Loading

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

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

Choose a reason for hiding this comment

jordansissel commented Oct 28, 2016

jordansissel commented Oct 28, 2016

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordansissel commented Nov 4, 2016

elasticsearch-bot commented Nov 4, 2016

andrewvc commented Oct 26, 2016 •

edited

Loading

suyograo Oct 28, 2016 •

edited

Loading