-
Notifications
You must be signed in to change notification settings - Fork 25.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ML] Datafeed config CRUD operations #32854
Changes from 1 commit
91ed28b
c989025
d979a0e
249c701
73ac690
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,6 +71,7 @@ public class DatafeedConfig extends AbstractDiffable<DatafeedConfig> implements | |
public static final String DOC_COUNT = "doc_count"; | ||
|
||
public static final ParseField ID = new ParseField("datafeed_id"); | ||
public static final ParseField CONFIG_TYPE = new ParseField("config_type"); | ||
public static final ParseField QUERY_DELAY = new ParseField("query_delay"); | ||
public static final ParseField FREQUENCY = new ParseField("frequency"); | ||
public static final ParseField INDEXES = new ParseField("indexes"); | ||
|
@@ -93,6 +94,7 @@ private static ObjectParser<Builder, Void> createParser(boolean ignoreUnknownFie | |
ObjectParser<Builder, Void> parser = new ObjectParser<>("datafeed_config", ignoreUnknownFields, Builder::new); | ||
|
||
parser.declareString(Builder::setId, ID); | ||
parser.declareString((c, s) -> {}, CONFIG_TYPE); | ||
parser.declareString(Builder::setJobId, Job.ID); | ||
parser.declareStringArray(Builder::setIndices, INDEXES); | ||
parser.declareStringArray(Builder::setIndices, INDICES); | ||
|
@@ -220,6 +222,10 @@ public String getJobId() { | |
return jobId; | ||
} | ||
|
||
public String getConfigType() { | ||
return TYPE; | ||
} | ||
|
||
public TimeValue getQueryDelay() { | ||
return queryDelay; | ||
} | ||
|
@@ -314,14 +320,14 @@ public void writeTo(StreamOutput out) throws IOException { | |
@Override | ||
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | ||
builder.startObject(); | ||
doXContentBody(builder, params); | ||
builder.endObject(); | ||
return builder; | ||
} | ||
|
||
public XContentBuilder doXContentBody(XContentBuilder builder, Params params) throws IOException { | ||
builder.field(ID.getPreferredName(), id); | ||
builder.field(Job.ID.getPreferredName(), jobId); | ||
// Config type field is added for the migration to index documents | ||
// and isn't needed in cluster state configs. Not writing the field | ||
// protects against BWC issues. | ||
if (params.paramAsBoolean(ToXContentParams.FOR_CLUSTER_STATE, false) == false) { | ||
builder.field(CONFIG_TYPE.getPreferredName(), TYPE); | ||
} | ||
builder.field(QUERY_DELAY.getPreferredName(), queryDelay.getStringRep()); | ||
if (frequency != null) { | ||
builder.field(FREQUENCY.getPreferredName(), frequency.getStringRep()); | ||
|
@@ -346,6 +352,7 @@ public XContentBuilder doXContentBody(XContentBuilder builder, Params params) th | |
if (headers.isEmpty() == false && params.paramAsBoolean(ToXContentParams.FOR_CLUSTER_STATE, false) == true) { | ||
builder.field(HEADERS.getPreferredName(), headers); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'll need to store the headers in the indexed document. |
||
} | ||
builder.endObject(); | ||
return builder; | ||
} | ||
|
||
|
@@ -485,6 +492,10 @@ public void setId(String datafeedId) { | |
id = ExceptionsHelper.requireNonNull(datafeedId, ID.getPreferredName()); | ||
} | ||
|
||
public String getId() { | ||
return id; | ||
} | ||
|
||
public void setJobId(String jobId) { | ||
this.jobId = ExceptionsHelper.requireNonNull(jobId, Job.ID.getPreferredName()); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,13 +8,17 @@ | |
import com.carrotsearch.randomizedtesting.generators.CodepointSetGenerator; | ||
|
||
import org.elasticsearch.ElasticsearchException; | ||
import org.elasticsearch.common.bytes.BytesReference; | ||
import org.elasticsearch.common.io.stream.NamedWriteableRegistry; | ||
import org.elasticsearch.common.io.stream.Writeable; | ||
import org.elasticsearch.common.settings.Settings; | ||
import org.elasticsearch.common.unit.TimeValue; | ||
import org.elasticsearch.common.xcontent.DeprecationHandler; | ||
import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; | ||
import org.elasticsearch.common.xcontent.NamedXContentRegistry; | ||
import org.elasticsearch.common.xcontent.ToXContent; | ||
import org.elasticsearch.common.xcontent.XContentFactory; | ||
import org.elasticsearch.common.xcontent.XContentHelper; | ||
import org.elasticsearch.common.xcontent.XContentParseException; | ||
import org.elasticsearch.common.xcontent.XContentParser; | ||
import org.elasticsearch.common.xcontent.XContentType; | ||
|
@@ -36,17 +40,22 @@ | |
import org.elasticsearch.test.ESTestCase; | ||
import org.elasticsearch.xpack.core.ml.datafeed.ChunkingConfig.Mode; | ||
import org.elasticsearch.xpack.core.ml.job.messages.Messages; | ||
import org.elasticsearch.xpack.core.ml.utils.ToXContentParams; | ||
import org.joda.time.DateTimeZone; | ||
|
||
import java.io.IOException; | ||
import java.util.ArrayList; | ||
import java.util.Collections; | ||
import java.util.HashMap; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.TimeZone; | ||
|
||
import static org.hamcrest.Matchers.containsString; | ||
import static org.hamcrest.Matchers.equalTo; | ||
import static org.hamcrest.Matchers.greaterThanOrEqualTo; | ||
import static org.hamcrest.Matchers.hasEntry; | ||
import static org.hamcrest.Matchers.hasSize; | ||
import static org.hamcrest.Matchers.is; | ||
import static org.hamcrest.Matchers.lessThan; | ||
import static org.hamcrest.Matchers.not; | ||
|
@@ -63,6 +72,10 @@ public static DatafeedConfig createRandomizedDatafeedConfig(String jobId) { | |
} | ||
|
||
public static DatafeedConfig createRandomizedDatafeedConfig(String jobId, long bucketSpanMillis) { | ||
return createRandomizedDatafeedConfigBuilder(jobId, bucketSpanMillis).build(); | ||
} | ||
|
||
private static DatafeedConfig.Builder createRandomizedDatafeedConfigBuilder(String jobId, long bucketSpanMillis) { | ||
DatafeedConfig.Builder builder = new DatafeedConfig.Builder(randomValidDatafeedId(), jobId); | ||
builder.setIndices(randomStringList(1, 10)); | ||
builder.setTypes(randomStringList(0, 10)); | ||
|
@@ -109,7 +122,7 @@ public static DatafeedConfig createRandomizedDatafeedConfig(String jobId, long b | |
if (randomBoolean()) { | ||
builder.setChunkingConfig(ChunkingConfigTests.createRandomizedChunk()); | ||
} | ||
return builder.build(); | ||
return builder; | ||
} | ||
|
||
@Override | ||
|
@@ -167,6 +180,33 @@ public void testFutureMetadataParse() throws IOException { | |
assertNotNull(DatafeedConfig.LENIENT_PARSER.apply(parser, null).build()); | ||
} | ||
|
||
public void testToXContentForClusterState() throws IOException { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted to add a test to check the datafeed type isn't written when we are writing to cluster state i.e. this piece of code in DatafeedConfig::toXContent
But it's hard to test so I added a check for the headers which similarly should only be persisted in certain circumstances There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not the following something similar to the following? It may work to simple directly check the serialized data string.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes that works, great idea I never thought of it. |
||
DatafeedConfig.Builder builder = createRandomizedDatafeedConfigBuilder("foo", 300); | ||
|
||
// headers are only persisted to cluster state | ||
Map<String, String> headers = new HashMap<>(); | ||
headers.put("header-name", "header-value"); | ||
builder.setHeaders(headers); | ||
DatafeedConfig config = builder.build(); | ||
|
||
ToXContent.MapParams params = new ToXContent.MapParams(Collections.singletonMap(ToXContentParams.FOR_CLUSTER_STATE, "true")); | ||
|
||
BytesReference forClusterstateXContent = XContentHelper.toXContent(config, XContentType.JSON, params, false); | ||
XContentParser parser = XContentFactory.xContent(XContentType.JSON) | ||
.createParser(xContentRegistry(), LoggingDeprecationHandler.INSTANCE, forClusterstateXContent.streamInput()); | ||
|
||
DatafeedConfig parsedConfig = DatafeedConfig.LENIENT_PARSER.apply(parser, null).build(); | ||
assertThat(parsedConfig.getHeaders(), hasEntry("header-name", "header-value")); | ||
|
||
// headers are not written without the FOR_CLUSTER_STATE param | ||
BytesReference nonClusterstateXContent = XContentHelper.toXContent(config, XContentType.JSON, ToXContent.EMPTY_PARAMS, false); | ||
parser = XContentFactory.xContent(XContentType.JSON) | ||
.createParser(xContentRegistry(), LoggingDeprecationHandler.INSTANCE, nonClusterstateXContent.streamInput()); | ||
|
||
parsedConfig = DatafeedConfig.LENIENT_PARSER.apply(parser, null).build(); | ||
assertThat(parsedConfig.getHeaders().entrySet(), hasSize(0)); | ||
} | ||
|
||
public void testCopyConstructor() { | ||
for (int i = 0; i < NUMBER_OF_TEST_RUNS; i++) { | ||
DatafeedConfig datafeedConfig = createTestInstance(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally this
ToXContentParams.FOR_CLUSTER_STATE
parameter was used to avoid showing the user internal implementation details. Based on the original intent the parameter should probably have been namedToXContentParams.FOR_INTERNAL_STORAGE
.There is also another constant,
MlMetaIndex.INCLUDE_TYPE_KEY
, which is used to avoid showing the end user the metadata document type in REST responses. I think we should also use that for config documents. So maybe moveINCLUDE_TYPE_KEY
intoToXContentParams
so it can be reused here.Then the logic here will be to include the type neither for cluster state nor for REST responses.