Make default target_bulk_bytes below AWS limit, but make it configurable #71
Conversation
Signed-off-by: Mårten Svantesson <[email protected]>
| # we won't use a lot of heap. | ||
| :target_bulk_bytes => { | ||
| :validate => :number, | ||
| :default => 9 * 1024 * 1024 # 9MiB |
There was a problem hiding this comment.
Why not keep default as 20MiB for backward compatible and allow AWS OpenSearch Service users to set value?
There was a problem hiding this comment.
Because it doesn't work for many users giving problems that are hard to resolve. I think it's better if the plugin works out of the box.
There was a problem hiding this comment.
Changing default value will break backwards compatibility. Correct me if I am wrong, but if a user is sending bulk requests to a self managed cluster configured with http.max_content_length > 9 MiB, upgrading to a version with this change would cause the plugin to fail to send all data.
Instead of changing default, maybe a better solution would be to update this documentation.
There was a problem hiding this comment.
Changing default value will break backwards compatibility. Correct me if I am wrong, but if a user is sending bulk requests to a self managed cluster configured with
http.max_content_length> 9 MiB, upgrading to a version with this change would cause the plugin to fail to send all data.
Not at all. The only drawback is a slightly lower maximum throughput. But with this fix a user can tweak to their hearts content.
There was a problem hiding this comment.
I see. Thanks for the correction.
Still, I feel this may violate backwards compatibility. If a user is sending bulk requests to a self managed cluster configured with http.max_content_length > 9 MiB, and they upgrade to this version, they may see performance degradation due to the decrease in maximum throughput. So, Im still in favor of keeping it at 20 MiB and letting users modify to work with AWS OpenSearch Service. The documentation above could be updated to prevent confusion.
There was a problem hiding this comment.
One problem i could see is that we might be flushing bulk request frequent than before for size > 9 and < 20. Let's say, my total request size is 18 MB, with this change it will send two request than one, and proposal is to configure the value to say 20MB @msvticket can you correct if my understanding is wrong?
There was a problem hiding this comment.
It's correct. My opinion is that it is more important to prevent errors than preserving probably not that common cases.
There was a problem hiding this comment.
I think the only thing you're arguing about is whether to increment a minor version for this change, or a major version.
Sounds like the default was poorly chosen. If it causes bugs, then it's a bug fix, and changing the value of the default in a minor version is OK. Otherwise do the change in a major version increment. My take for this case is that it's ok to do it in a minor version increment.
Since it's configurable, you are preserving backwards compatibility. Behavior may change, but it's not broken. You will surprise some people who didn't read the release notes.
VijayanB
added
the
enhancement
| # we won't use a lot of heap. | ||
| :target_bulk_bytes => { | ||
| :validate => :number, | ||
| :default => 9 * 1024 * 1024 # 9MiB |
There was a problem hiding this comment.
I think the only thing you're arguing about is whether to increment a minor version for this change, or a major version.
Sounds like the default was poorly chosen. If it causes bugs, then it's a bug fix, and changing the value of the default in a minor version is OK. Otherwise do the change in a major version increment. My take for this case is that it's ok to do it in a minor version increment.
Since it's configurable, you are preserving backwards compatibility. Behavior may change, but it's not broken. You will surprise some people who didn't read the release notes.
| @@ -35,6 +35,7 @@ def self.build(logger, hosts, params) | |||
| end | |||
|
|
|||
| common_options[:timeout] = params["timeout"] if params["timeout"] | |||
There was a problem hiding this comment.
While we are here, does specifying timeout = nil make the request not timeout? For another PR/workitem I'm used to seeing if params.key?(:timeout) and support all values with predictable behavior.
|
Thank you @msvticket for your contribution and looking forward for more. We will release this feature as part of our next release (1.2.0) . |
Description
This fix prevents http status 413
Fixes old bug carrying over from logstash-output-elasticsearch: logstash-plugins/logstash-output-elasticsearch#785
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.