From 26a71266368054ffb6e441e0d4d786b415580138 Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Wed, 12 Sep 2018 22:02:16 +0200 Subject: [PATCH 1/2] [CCR] Add validation for max_retry_delay --- .../ccr/action/FollowIndexRequestTests.java | 22 ++++++++++ .../core/ccr/action/FollowIndexAction.java | 23 +++++++++-- .../action/PutAutoFollowPatternAction.java | 11 +++++ .../PutAutoFollowPatternRequestTests.java | 41 +++++++++++++++++++ 4 files changed, 93 insertions(+), 4 deletions(-) diff --git a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/FollowIndexRequestTests.java b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/FollowIndexRequestTests.java index 2017fa2fdb989..afe303720a38d 100644 --- a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/FollowIndexRequestTests.java +++ b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/FollowIndexRequestTests.java @@ -5,6 +5,7 @@ */ package org.elasticsearch.xpack.ccr.action; +import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.test.AbstractStreamableXContentTestCase; @@ -12,6 +13,10 @@ import java.io.IOException; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.nullValue; + public class FollowIndexRequestTests extends AbstractStreamableXContentTestCase { @Override @@ -39,4 +44,21 @@ static FollowIndexAction.Request createTestRequest() { randomIntBetween(1, Integer.MAX_VALUE), randomNonNegativeLong(), randomIntBetween(1, Integer.MAX_VALUE), randomIntBetween(1, Integer.MAX_VALUE), TimeValue.timeValueMillis(500), TimeValue.timeValueMillis(500)); } + + public void testValidate() { + FollowIndexAction.Request request = new FollowIndexAction.Request("index1", "index2", null, null, null, null, + null, TimeValue.ZERO, null); + ActionRequestValidationException validationException = request.validate(); + assertThat(validationException, notNullValue()); + assertThat(validationException.getMessage(), containsString("maxRetryDelay must be positive but was [0ms]")); + + request = new FollowIndexAction.Request("index1", "index2", null, null, null, null, null, TimeValue.timeValueMinutes(10), null); + validationException = request.validate(); + assertThat(validationException, notNullValue()); + assertThat(validationException.getMessage(), containsString("maxRetryDelay must be less than [5m] but was [10m]")); + + request = new FollowIndexAction.Request("index1", "index2", null, null, null, null, null, TimeValue.timeValueMinutes(1), null); + validationException = request.validate(); + assertThat(validationException, nullValue()); + } } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/action/FollowIndexAction.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/action/FollowIndexAction.java index 2c311356d4943..cc0bb23874557 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/action/FollowIndexAction.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/action/FollowIndexAction.java @@ -23,6 +23,8 @@ import java.io.IOException; import java.util.Objects; +import static org.elasticsearch.action.ValidateActions.addValidationError; + public final class FollowIndexAction extends Action { public static final FollowIndexAction INSTANCE = new FollowIndexAction(); @@ -33,8 +35,9 @@ public final class FollowIndexAction extends Action { public static final int DEFAULT_MAX_CONCURRENT_READ_BATCHES = 1; public static final int DEFAULT_MAX_CONCURRENT_WRITE_BATCHES = 1; public static final long DEFAULT_MAX_BATCH_SIZE_IN_BYTES = Long.MAX_VALUE; - public static final TimeValue DEFAULT_RETRY_TIMEOUT = new TimeValue(500); - public static final TimeValue DEFAULT_IDLE_SHARD_RETRY_DELAY = TimeValue.timeValueSeconds(10); + static final TimeValue DEFAULT_MAX_RETRY_DELAY = new TimeValue(500); + static final TimeValue DEFAULT_IDLE_SHARD_RETRY_DELAY = TimeValue.timeValueSeconds(10); + static final TimeValue MAX_MAX_RETRY_DELAY = TimeValue.timeValueMinutes(5); private FollowIndexAction() { super(NAME); @@ -202,7 +205,7 @@ public Request( throw new IllegalArgumentException(MAX_WRITE_BUFFER_SIZE.getPreferredName() + " must be larger than 0"); } - final TimeValue actualRetryTimeout = maxRetryDelay == null ? DEFAULT_RETRY_TIMEOUT : maxRetryDelay; + final TimeValue actualRetryTimeout = maxRetryDelay == null ? DEFAULT_MAX_RETRY_DELAY : maxRetryDelay; final TimeValue actualIdleShardRetryDelay = idleShardRetryDelay == null ? DEFAULT_IDLE_SHARD_RETRY_DELAY : idleShardRetryDelay; this.leaderIndex = leaderIndex; @@ -222,7 +225,19 @@ public Request() { @Override public ActionRequestValidationException validate() { - return null; + ActionRequestValidationException validationException = null; + + if (maxRetryDelay.millis() <= 0) { + String message = "maxRetryDelay must be positive but was [" + maxRetryDelay.getStringRep() + "]"; + validationException = addValidationError(message, validationException); + } + if (maxRetryDelay.millis() > MAX_MAX_RETRY_DELAY.millis()) { + String message = "maxRetryDelay must be less than [" + FollowIndexAction.MAX_MAX_RETRY_DELAY + + "] but was [" + maxRetryDelay.getStringRep() + "]"; + validationException = addValidationError(message, validationException); + } + + return validationException; } @Override diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/action/PutAutoFollowPatternAction.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/action/PutAutoFollowPatternAction.java index dc69795bb4a01..2043c8f5af243 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/action/PutAutoFollowPatternAction.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/action/PutAutoFollowPatternAction.java @@ -99,6 +99,17 @@ public ActionRequestValidationException validate() { if (leaderIndexPatterns == null || leaderIndexPatterns.isEmpty()) { validationException = addValidationError("leaderIndexPatterns is missing", validationException); } + if (maxRetryDelay != null) { + if (maxRetryDelay.millis() <= 0) { + String message = "maxRetryDelay must be positive but was [" + maxRetryDelay.getStringRep() + "]"; + validationException = addValidationError(message, validationException); + } + if (maxRetryDelay.millis() > FollowIndexAction.MAX_MAX_RETRY_DELAY.millis()) { + String message = "maxRetryDelay must be less than [" + FollowIndexAction.MAX_MAX_RETRY_DELAY + + "] but was [" + maxRetryDelay.getStringRep() + "]"; + validationException = addValidationError(message, validationException); + } + } return validationException; } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ccr/action/PutAutoFollowPatternRequestTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ccr/action/PutAutoFollowPatternRequestTests.java index f11e1885e80e3..24508c329396b 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ccr/action/PutAutoFollowPatternRequestTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ccr/action/PutAutoFollowPatternRequestTests.java @@ -5,12 +5,18 @@ */ package org.elasticsearch.xpack.core.ccr.action; +import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.test.AbstractStreamableXContentTestCase; import java.io.IOException; import java.util.Arrays; +import java.util.Collections; + +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.nullValue; public class PutAutoFollowPatternRequestTests extends AbstractStreamableXContentTestCase { @@ -60,4 +66,39 @@ protected PutAutoFollowPatternAction.Request createTestInstance() { } return request; } + + public void testValidate() { + PutAutoFollowPatternAction.Request request = new PutAutoFollowPatternAction.Request(); + ActionRequestValidationException validationException = request.validate(); + assertThat(validationException, notNullValue()); + assertThat(validationException.getMessage(), containsString("leaderClusterAlias is missing")); + + request.setLeaderClusterAlias("_alias"); + validationException = request.validate(); + assertThat(validationException, notNullValue()); + assertThat(validationException.getMessage(), containsString("leaderIndexPatterns is missing")); + + request.setLeaderIndexPatterns(Collections.emptyList()); + validationException = request.validate(); + assertThat(validationException, notNullValue()); + assertThat(validationException.getMessage(), containsString("leaderIndexPatterns is missing")); + + request.setLeaderIndexPatterns(Collections.singletonList("logs-*")); + validationException = request.validate(); + assertThat(validationException, nullValue()); + + request.setMaxRetryDelay(TimeValue.ZERO); + validationException = request.validate(); + assertThat(validationException, notNullValue()); + assertThat(validationException.getMessage(), containsString("maxRetryDelay must be positive but was [0ms]")); + + request.setMaxRetryDelay(TimeValue.timeValueMinutes(10)); + validationException = request.validate(); + assertThat(validationException, notNullValue()); + assertThat(validationException.getMessage(), containsString("maxRetryDelay must be less than [5m] but was [10m]")); + + request.setMaxRetryDelay(TimeValue.timeValueMinutes(1)); + validationException = request.validate(); + assertThat(validationException, nullValue()); + } } From 6a333f7c87ccff55e26656ae6684540dddaa46c7 Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Thu, 13 Sep 2018 08:55:26 +0200 Subject: [PATCH 2/2] iter --- .../ccr/action/FollowIndexRequestTests.java | 4 ++-- .../xpack/core/ccr/AutoFollowMetadata.java | 2 +- .../core/ccr/action/FollowIndexAction.java | 17 +++++++++-------- .../ccr/action/PutAutoFollowPatternAction.java | 14 +++++++++----- .../PutAutoFollowPatternRequestTests.java | 10 +++++----- 5 files changed, 26 insertions(+), 21 deletions(-) diff --git a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/FollowIndexRequestTests.java b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/FollowIndexRequestTests.java index afe303720a38d..e5f7e693a7f1c 100644 --- a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/FollowIndexRequestTests.java +++ b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/FollowIndexRequestTests.java @@ -50,12 +50,12 @@ public void testValidate() { null, TimeValue.ZERO, null); ActionRequestValidationException validationException = request.validate(); assertThat(validationException, notNullValue()); - assertThat(validationException.getMessage(), containsString("maxRetryDelay must be positive but was [0ms]")); + assertThat(validationException.getMessage(), containsString("[max_retry_delay] must be positive but was [0ms]")); request = new FollowIndexAction.Request("index1", "index2", null, null, null, null, null, TimeValue.timeValueMinutes(10), null); validationException = request.validate(); assertThat(validationException, notNullValue()); - assertThat(validationException.getMessage(), containsString("maxRetryDelay must be less than [5m] but was [10m]")); + assertThat(validationException.getMessage(), containsString("[max_retry_delay] must be less than [5m] but was [10m]")); request = new FollowIndexAction.Request("index1", "index2", null, null, null, null, null, TimeValue.timeValueMinutes(1), null); validationException = request.validate(); diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/AutoFollowMetadata.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/AutoFollowMetadata.java index 71fd13d0b504f..9c64ea3da764c 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/AutoFollowMetadata.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/AutoFollowMetadata.java @@ -169,7 +169,7 @@ public static class AutoFollowPattern implements Writeable, ToXContentObject { public static final ParseField MAX_BATCH_SIZE_IN_BYTES = new ParseField("max_batch_size_in_bytes"); public static final ParseField MAX_CONCURRENT_WRITE_BATCHES = new ParseField("max_concurrent_write_batches"); public static final ParseField MAX_WRITE_BUFFER_SIZE = new ParseField("max_write_buffer_size"); - public static final ParseField MAX_RETRY_DELAY = new ParseField("retry_timeout"); + public static final ParseField MAX_RETRY_DELAY = new ParseField("max_retry_delay"); public static final ParseField IDLE_SHARD_RETRY_DELAY = new ParseField("idle_shard_retry_delay"); @SuppressWarnings("unchecked") diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/action/FollowIndexAction.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/action/FollowIndexAction.java index cc0bb23874557..d5a0b0408c565 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/action/FollowIndexAction.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/action/FollowIndexAction.java @@ -37,7 +37,7 @@ public final class FollowIndexAction extends Action { public static final long DEFAULT_MAX_BATCH_SIZE_IN_BYTES = Long.MAX_VALUE; static final TimeValue DEFAULT_MAX_RETRY_DELAY = new TimeValue(500); static final TimeValue DEFAULT_IDLE_SHARD_RETRY_DELAY = TimeValue.timeValueSeconds(10); - static final TimeValue MAX_MAX_RETRY_DELAY = TimeValue.timeValueMinutes(5); + static final TimeValue MAX_RETRY_DELAY = TimeValue.timeValueMinutes(5); private FollowIndexAction() { super(NAME); @@ -57,7 +57,7 @@ public static class Request extends ActionRequest implements ToXContentObject { private static final ParseField MAX_BATCH_SIZE_IN_BYTES = new ParseField("max_batch_size_in_bytes"); private static final ParseField MAX_CONCURRENT_WRITE_BATCHES = new ParseField("max_concurrent_write_batches"); private static final ParseField MAX_WRITE_BUFFER_SIZE = new ParseField("max_write_buffer_size"); - private static final ParseField MAX_RETRY_DELAY = new ParseField("max_retry_delay"); + private static final ParseField MAX_RETRY_DELAY_FIELD = new ParseField("max_retry_delay"); private static final ParseField IDLE_SHARD_RETRY_DELAY = new ParseField("idle_shard_retry_delay"); private static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>(NAME, true, (args, followerIndex) -> { @@ -78,8 +78,8 @@ public static class Request extends ActionRequest implements ToXContentObject { PARSER.declareInt(ConstructingObjectParser.optionalConstructorArg(), MAX_WRITE_BUFFER_SIZE); PARSER.declareField( ConstructingObjectParser.optionalConstructorArg(), - (p, c) -> TimeValue.parseTimeValue(p.text(), MAX_RETRY_DELAY.getPreferredName()), - MAX_RETRY_DELAY, + (p, c) -> TimeValue.parseTimeValue(p.text(), MAX_RETRY_DELAY_FIELD.getPreferredName()), + MAX_RETRY_DELAY_FIELD, ObjectParser.ValueType.STRING); PARSER.declareField( ConstructingObjectParser.optionalConstructorArg(), @@ -228,11 +228,12 @@ public ActionRequestValidationException validate() { ActionRequestValidationException validationException = null; if (maxRetryDelay.millis() <= 0) { - String message = "maxRetryDelay must be positive but was [" + maxRetryDelay.getStringRep() + "]"; + String message = "[" + MAX_RETRY_DELAY_FIELD.getPreferredName() + "] must be positive but was [" + + maxRetryDelay.getStringRep() + "]"; validationException = addValidationError(message, validationException); } - if (maxRetryDelay.millis() > MAX_MAX_RETRY_DELAY.millis()) { - String message = "maxRetryDelay must be less than [" + FollowIndexAction.MAX_MAX_RETRY_DELAY + + if (maxRetryDelay.millis() > FollowIndexAction.MAX_RETRY_DELAY.millis()) { + String message = "[" + MAX_RETRY_DELAY_FIELD.getPreferredName() + "] must be less than [" + MAX_RETRY_DELAY + "] but was [" + maxRetryDelay.getStringRep() + "]"; validationException = addValidationError(message, validationException); } @@ -279,7 +280,7 @@ public XContentBuilder toXContent(final XContentBuilder builder, final Params pa builder.field(MAX_WRITE_BUFFER_SIZE.getPreferredName(), maxWriteBufferSize); builder.field(MAX_CONCURRENT_READ_BATCHES.getPreferredName(), maxConcurrentReadBatches); builder.field(MAX_CONCURRENT_WRITE_BATCHES.getPreferredName(), maxConcurrentWriteBatches); - builder.field(MAX_RETRY_DELAY.getPreferredName(), maxRetryDelay.getStringRep()); + builder.field(MAX_RETRY_DELAY_FIELD.getPreferredName(), maxRetryDelay.getStringRep()); builder.field(IDLE_SHARD_RETRY_DELAY.getPreferredName(), idleShardRetryDelay.getStringRep()); } builder.endObject(); diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/action/PutAutoFollowPatternAction.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/action/PutAutoFollowPatternAction.java index 2043c8f5af243..01ebd3f1d81f1 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/action/PutAutoFollowPatternAction.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/action/PutAutoFollowPatternAction.java @@ -94,18 +94,22 @@ public static Request fromXContent(XContentParser parser, String remoteClusterAl public ActionRequestValidationException validate() { ActionRequestValidationException validationException = null; if (leaderClusterAlias == null) { - validationException = addValidationError("leaderClusterAlias is missing", validationException); + validationException = addValidationError("[" + LEADER_CLUSTER_ALIAS_FIELD.getPreferredName() + + "] is missing", validationException); } if (leaderIndexPatterns == null || leaderIndexPatterns.isEmpty()) { - validationException = addValidationError("leaderIndexPatterns is missing", validationException); + validationException = addValidationError("[" + LEADER_INDEX_PATTERNS_FIELD.getPreferredName() + + "] is missing", validationException); } if (maxRetryDelay != null) { if (maxRetryDelay.millis() <= 0) { - String message = "maxRetryDelay must be positive but was [" + maxRetryDelay.getStringRep() + "]"; + String message = "[" + AutoFollowPattern.MAX_RETRY_DELAY.getPreferredName() + "] must be positive but was [" + + maxRetryDelay.getStringRep() + "]"; validationException = addValidationError(message, validationException); } - if (maxRetryDelay.millis() > FollowIndexAction.MAX_MAX_RETRY_DELAY.millis()) { - String message = "maxRetryDelay must be less than [" + FollowIndexAction.MAX_MAX_RETRY_DELAY + + if (maxRetryDelay.millis() > FollowIndexAction.MAX_RETRY_DELAY.millis()) { + String message = "[" + AutoFollowPattern.MAX_RETRY_DELAY.getPreferredName() + "] must be less than [" + + FollowIndexAction.MAX_RETRY_DELAY + "] but was [" + maxRetryDelay.getStringRep() + "]"; validationException = addValidationError(message, validationException); } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ccr/action/PutAutoFollowPatternRequestTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ccr/action/PutAutoFollowPatternRequestTests.java index 24508c329396b..ced49bbae128a 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ccr/action/PutAutoFollowPatternRequestTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ccr/action/PutAutoFollowPatternRequestTests.java @@ -71,17 +71,17 @@ public void testValidate() { PutAutoFollowPatternAction.Request request = new PutAutoFollowPatternAction.Request(); ActionRequestValidationException validationException = request.validate(); assertThat(validationException, notNullValue()); - assertThat(validationException.getMessage(), containsString("leaderClusterAlias is missing")); + assertThat(validationException.getMessage(), containsString("[leader_cluster_alias] is missing")); request.setLeaderClusterAlias("_alias"); validationException = request.validate(); assertThat(validationException, notNullValue()); - assertThat(validationException.getMessage(), containsString("leaderIndexPatterns is missing")); + assertThat(validationException.getMessage(), containsString("[leader_index_patterns] is missing")); request.setLeaderIndexPatterns(Collections.emptyList()); validationException = request.validate(); assertThat(validationException, notNullValue()); - assertThat(validationException.getMessage(), containsString("leaderIndexPatterns is missing")); + assertThat(validationException.getMessage(), containsString("[leader_index_patterns] is missing")); request.setLeaderIndexPatterns(Collections.singletonList("logs-*")); validationException = request.validate(); @@ -90,12 +90,12 @@ public void testValidate() { request.setMaxRetryDelay(TimeValue.ZERO); validationException = request.validate(); assertThat(validationException, notNullValue()); - assertThat(validationException.getMessage(), containsString("maxRetryDelay must be positive but was [0ms]")); + assertThat(validationException.getMessage(), containsString("[max_retry_delay] must be positive but was [0ms]")); request.setMaxRetryDelay(TimeValue.timeValueMinutes(10)); validationException = request.validate(); assertThat(validationException, notNullValue()); - assertThat(validationException.getMessage(), containsString("maxRetryDelay must be less than [5m] but was [10m]")); + assertThat(validationException.getMessage(), containsString("[max_retry_delay] must be less than [5m] but was [10m]")); request.setMaxRetryDelay(TimeValue.timeValueMinutes(1)); validationException = request.validate();