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@9be194006e
This commit is contained in:
Tal Levy 2018-01-31 09:13:46 -08:00 committed by GitHub
parent c20f3ba996
commit a627fec53e
5 changed files with 18 additions and 14 deletions

View File

@ -85,7 +85,7 @@ public class HttpExporter extends Exporter {
*/ */
public static final Setting.AffixSetting<TimeValue> BULK_TIMEOUT_SETTING = public static final Setting.AffixSetting<TimeValue> BULK_TIMEOUT_SETTING =
Setting.affixKeySetting("xpack.monitoring.exporters.","bulk.timeout", 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. * Timeout used for initiating a connection.
*/ */
@ -147,7 +147,7 @@ public class HttpExporter extends Exporter {
*/ */
public static final Setting.AffixSetting<TimeValue> TEMPLATE_CHECK_TIMEOUT_SETTING = public static final Setting.AffixSetting<TimeValue> TEMPLATE_CHECK_TIMEOUT_SETTING =
Setting.affixKeySetting("xpack.monitoring.exporters.","index.template.master_timeout", 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. * 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<TimeValue> PIPELINE_CHECK_TIMEOUT_SETTING = public static final Setting.AffixSetting<TimeValue> PIPELINE_CHECK_TIMEOUT_SETTING =
Setting.affixKeySetting("xpack.monitoring.exporters.","index.pipeline.master_timeout", 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). * Minimum supported version of the remote monitoring cluster (same major).
@ -517,8 +517,8 @@ public class HttpExporter extends Exporter {
final MapBuilder<String, String> params = new MapBuilder<>(); final MapBuilder<String, String> params = new MapBuilder<>();
if (bulkTimeout != null) { if (TimeValue.MINUS_ONE.equals(bulkTimeout) == false) {
params.put("master_timeout", bulkTimeout.toString()); params.put("timeout", bulkTimeout.toString());
} }
// allow the use of ingest pipelines to be completely optional // allow the use of ingest pipelines to be completely optional

View File

@ -108,7 +108,7 @@ public abstract class PublishableHttpResource extends HttpResource {
* Create a new {@link PublishableHttpResource}. * Create a new {@link PublishableHttpResource}.
* *
* @param resourceOwnerName The user-recognizable name. * @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 baseParameters The base parameters to specify for the request.
* @param dirty Whether the resource is dirty or not * @param dirty Whether the resource is dirty or not
*/ */
@ -116,7 +116,7 @@ public abstract class PublishableHttpResource extends HttpResource {
final Map<String, String> baseParameters, final boolean dirty) { final Map<String, String> baseParameters, final boolean dirty) {
super(resourceOwnerName, dirty); super(resourceOwnerName, dirty);
if (masterTimeout != null) { if (masterTimeout != null && TimeValue.MINUS_ONE.equals(masterTimeout) == false) {
final Map<String, String> parameters = new HashMap<>(baseParameters.size() + 1); final Map<String, String> parameters = new HashMap<>(baseParameters.size() + 1);
parameters.putAll(baseParameters); parameters.putAll(baseParameters);

View File

@ -43,7 +43,7 @@ public abstract class AbstractPublishableHttpResourceTestCase extends ESTestCase
protected final String owner = getClass().getSimpleName(); protected final String owner = getClass().getSimpleName();
@Nullable @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); protected final RestClient client = mock(RestClient.class);
@ -182,8 +182,10 @@ public abstract class AbstractPublishableHttpResourceTestCase extends ESTestCase
protected void assertParameters(final PublishableHttpResource resource) { protected void assertParameters(final PublishableHttpResource resource) {
final Map<String, String> parameters = new HashMap<>(resource.getParameters()); final Map<String, String> 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())); assertThat(parameters.remove("master_timeout"), is(masterTimeout.toString()));
} else {
assertFalse(parameters.containsKey("master_timeout"));
} }
assertThat(parameters.remove("filter_path"), is("$NONE")); assertThat(parameters.remove("filter_path"), is("$NONE"));
@ -193,8 +195,10 @@ public abstract class AbstractPublishableHttpResourceTestCase extends ESTestCase
protected void assertVersionParameters(final PublishableHttpResource resource) { protected void assertVersionParameters(final PublishableHttpResource resource) {
final Map<String, String> parameters = new HashMap<>(resource.getParameters()); final Map<String, String> 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())); assertThat(parameters.remove("master_timeout"), is(masterTimeout.toString()));
} else {
assertFalse(parameters.containsKey("master_timeout"));
} }
assertThat(parameters.remove("filter_path"), is("*.version")); assertThat(parameters.remove("filter_path"), is("*.version"));

View File

@ -626,7 +626,7 @@ public class HttpExporterIT extends MonitoringIntegTestCase {
} }
private String resourceVersionQueryString() { private String resourceVersionQueryString() {
return "master_timeout=10s&filter_path=" + FILTER_PATH_RESOURCE_VERSION; return "filter_path=" + FILTER_PATH_RESOURCE_VERSION;
} }
private String watcherCheckQueryString() { private String watcherCheckQueryString() {
@ -636,7 +636,7 @@ public class HttpExporterIT extends MonitoringIntegTestCase {
private String bulkQueryString() { private String bulkQueryString() {
final String pipelineName = MonitoringTemplateUtils.pipelineName(TEMPLATE_VERSION); 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 { private void enqueueGetClusterVersionResponse(Version v) throws IOException {

View File

@ -416,9 +416,9 @@ public class HttpExporterTests extends ESTestCase {
assertThat(parameters.remove("filter_path"), equalTo("errors,items.*.error")); assertThat(parameters.remove("filter_path"), equalTo("errors,items.*.error"));
if (bulkTimeout != null) { if (bulkTimeout != null) {
assertThat(parameters.remove("master_timeout"), equalTo(bulkTimeout.toString())); assertThat(parameters.remove("timeout"), equalTo(bulkTimeout.toString()));
} else { } else {
assertThat(parameters.remove("master_timeout"), equalTo("10s")); assertNull(parameters.remove("timeout"));
} }
if (useIngest) { if (useIngest) {