From a627fec53e717755ab63e8383d9a091d879ce56f Mon Sep 17 00:00:00 2001 From: Tal Levy Date: Wed, 31 Jan 2018 09:13:46 -0800 Subject: [PATCH] fix timeout usage in _bulk to `timeout` from `master_timeout` (elastic/x-pack-elasticsearch#3796) The HTTP Exporter in Monitoring allowed users to set a timeout parameters for the requests. When set, this was setting the `master_timeout` query parameter in Bulk Requests. The problem is that Bulk Requests do not support this type of timeout. Original commit: elastic/x-pack-elasticsearch@9be194006e9b9f91d9d53df7ac56d2e546306963 --- .../xpack/monitoring/exporter/http/HttpExporter.java | 10 +++++----- .../exporter/http/PublishableHttpResource.java | 4 ++-- .../http/AbstractPublishableHttpResourceTestCase.java | 10 +++++++--- .../xpack/monitoring/exporter/http/HttpExporterIT.java | 4 ++-- .../monitoring/exporter/http/HttpExporterTests.java | 4 ++-- 5 files changed, 18 insertions(+), 14 deletions(-) diff --git a/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java b/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java index 982b271610b..42f29a83a81 100644 --- a/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java +++ b/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporter.java @@ -85,7 +85,7 @@ public class HttpExporter extends Exporter { */ public static final Setting.AffixSetting BULK_TIMEOUT_SETTING = Setting.affixKeySetting("xpack.monitoring.exporters.","bulk.timeout", - (key) -> Setting.timeSetting(key, TimeValue.timeValueSeconds(10), Property.Dynamic, Property.NodeScope)); + (key) -> Setting.timeSetting(key, TimeValue.MINUS_ONE, Property.Dynamic, Property.NodeScope)); /** * Timeout used for initiating a connection. */ @@ -147,7 +147,7 @@ public class HttpExporter extends Exporter { */ public static final Setting.AffixSetting TEMPLATE_CHECK_TIMEOUT_SETTING = Setting.affixKeySetting("xpack.monitoring.exporters.","index.template.master_timeout", - (key) -> Setting.timeSetting(key, TimeValue.timeValueSeconds(10), Property.Dynamic, Property.NodeScope)); + (key) -> Setting.timeSetting(key, TimeValue.MINUS_ONE, Property.Dynamic, Property.NodeScope)); /** * A boolean setting to enable or disable whether to create placeholders for the old templates. */ @@ -159,7 +159,7 @@ public class HttpExporter extends Exporter { */ public static final Setting.AffixSetting PIPELINE_CHECK_TIMEOUT_SETTING = Setting.affixKeySetting("xpack.monitoring.exporters.","index.pipeline.master_timeout", - (key) -> Setting.timeSetting(key, TimeValue.timeValueSeconds(10), Property.Dynamic, Property.NodeScope)); + (key) -> Setting.timeSetting(key, TimeValue.MINUS_ONE, Property.Dynamic, Property.NodeScope)); /** * Minimum supported version of the remote monitoring cluster (same major). @@ -517,8 +517,8 @@ public class HttpExporter extends Exporter { final MapBuilder params = new MapBuilder<>(); - if (bulkTimeout != null) { - params.put("master_timeout", bulkTimeout.toString()); + if (TimeValue.MINUS_ONE.equals(bulkTimeout) == false) { + params.put("timeout", bulkTimeout.toString()); } // allow the use of ingest pipelines to be completely optional diff --git a/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/PublishableHttpResource.java b/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/PublishableHttpResource.java index 5ec5c0d1e56..168ccff44c4 100644 --- a/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/PublishableHttpResource.java +++ b/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/http/PublishableHttpResource.java @@ -108,7 +108,7 @@ public abstract class PublishableHttpResource extends HttpResource { * Create a new {@link PublishableHttpResource}. * * @param resourceOwnerName The user-recognizable name. - * @param masterTimeout Master timeout to use with any request. + * @param masterTimeout timeout to use with any request. * @param baseParameters The base parameters to specify for the request. * @param dirty Whether the resource is dirty or not */ @@ -116,7 +116,7 @@ public abstract class PublishableHttpResource extends HttpResource { final Map baseParameters, final boolean dirty) { super(resourceOwnerName, dirty); - if (masterTimeout != null) { + if (masterTimeout != null && TimeValue.MINUS_ONE.equals(masterTimeout) == false) { final Map parameters = new HashMap<>(baseParameters.size() + 1); parameters.putAll(baseParameters); diff --git a/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/AbstractPublishableHttpResourceTestCase.java b/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/AbstractPublishableHttpResourceTestCase.java index 9288b809a71..37b59ddcdc1 100644 --- a/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/AbstractPublishableHttpResourceTestCase.java +++ b/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/AbstractPublishableHttpResourceTestCase.java @@ -43,7 +43,7 @@ public abstract class AbstractPublishableHttpResourceTestCase extends ESTestCase protected final String owner = getClass().getSimpleName(); @Nullable - protected final TimeValue masterTimeout = randomFrom(TimeValue.timeValueMinutes(5), null); + protected final TimeValue masterTimeout = randomFrom(TimeValue.timeValueMinutes(5), TimeValue.MINUS_ONE, null); protected final RestClient client = mock(RestClient.class); @@ -182,8 +182,10 @@ public abstract class AbstractPublishableHttpResourceTestCase extends ESTestCase protected void assertParameters(final PublishableHttpResource resource) { final Map parameters = new HashMap<>(resource.getParameters()); - if (masterTimeout != null) { + if (masterTimeout != null && TimeValue.MINUS_ONE.equals(masterTimeout) == false) { assertThat(parameters.remove("master_timeout"), is(masterTimeout.toString())); + } else { + assertFalse(parameters.containsKey("master_timeout")); } assertThat(parameters.remove("filter_path"), is("$NONE")); @@ -193,8 +195,10 @@ public abstract class AbstractPublishableHttpResourceTestCase extends ESTestCase protected void assertVersionParameters(final PublishableHttpResource resource) { final Map parameters = new HashMap<>(resource.getParameters()); - if (masterTimeout != null) { + if (masterTimeout != null && TimeValue.MINUS_ONE.equals(masterTimeout) == false) { assertThat(parameters.remove("master_timeout"), is(masterTimeout.toString())); + } else { + assertFalse(parameters.containsKey("master_timeout")); } assertThat(parameters.remove("filter_path"), is("*.version")); diff --git a/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterIT.java b/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterIT.java index 531423a9b41..a23755cfaf1 100644 --- a/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterIT.java +++ b/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterIT.java @@ -626,7 +626,7 @@ public class HttpExporterIT extends MonitoringIntegTestCase { } private String resourceVersionQueryString() { - return "master_timeout=10s&filter_path=" + FILTER_PATH_RESOURCE_VERSION; + return "filter_path=" + FILTER_PATH_RESOURCE_VERSION; } private String watcherCheckQueryString() { @@ -636,7 +636,7 @@ public class HttpExporterIT extends MonitoringIntegTestCase { private String bulkQueryString() { final String pipelineName = MonitoringTemplateUtils.pipelineName(TEMPLATE_VERSION); - return "master_timeout=10s&pipeline=" + pipelineName + "&filter_path=" + "errors,items.*.error"; + return "pipeline=" + pipelineName + "&filter_path=" + "errors,items.*.error"; } private void enqueueGetClusterVersionResponse(Version v) throws IOException { diff --git a/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterTests.java b/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterTests.java index d524a4aa7ef..52eed801b32 100644 --- a/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterTests.java +++ b/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterTests.java @@ -416,9 +416,9 @@ public class HttpExporterTests extends ESTestCase { assertThat(parameters.remove("filter_path"), equalTo("errors,items.*.error")); if (bulkTimeout != null) { - assertThat(parameters.remove("master_timeout"), equalTo(bulkTimeout.toString())); + assertThat(parameters.remove("timeout"), equalTo(bulkTimeout.toString())); } else { - assertThat(parameters.remove("master_timeout"), equalTo("10s")); + assertNull(parameters.remove("timeout")); } if (useIngest) {