Skip to content
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.

Sign up for GitHub

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] Delete forecast API (#31134) #33218

Merged
merged 12 commits into from
Sep 4, 2018
Merged

[ML] Delete forecast API (#31134) #33218

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d40b201
Delete forecast API (#31134)
benwtrent Aug 28, 2018
ec318a3
Merge branch 'master' into feature/delete-forecast-api
benwtrent Aug 29, 2018
c6d3869
Adding ability to delete more than one forecast
benwtrent Aug 29, 2018
283f3fc
minor changes and adding yaml tests
benwtrent Aug 30, 2018
70cdfaf
Fixing yaml tests, adjusting search and delete
benwtrent Aug 31, 2018
96c53e1
Merge branch 'master' into feature/delete-forecast-api
benwtrent Aug 31, 2018
cd7b37c
Merge branch 'master' into feature/delete-forecast-api
benwtrent Aug 31, 2018
08a32bf
making stream and parser try-with-resource
benwtrent Sep 3, 2018
d170da4
Merge branch 'master' into feature/delete-forecast-api
benwtrent Sep 3, 2018
de0bead
Merge branch 'master' into feature/delete-forecast-api
benwtrent Sep 3, 2018
eccab09
blacklisting tests that are meant to raise errors
benwtrent Sep 3, 2018
8643d1a
Removing forbidden API utilization
benwtrent Sep 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions ...lugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/DeleteForecastAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,14 @@
import org.elasticsearch.action.ActionRequest;
import org.elasticsearch.action.ActionRequestBuilder;
import org.elasticsearch.action.ActionRequestValidationException;
import org.elasticsearch.action.support.master.AcknowledgedRequest;
import org.elasticsearch.action.support.master.AcknowledgedResponse;
import org.elasticsearch.client.ElasticsearchClient;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.xpack.core.ml.job.config.Job;
import org.elasticsearch.xpack.core.ml.job.process.autodetect.state.ModelSnapshotField;
import org.elasticsearch.xpack.core.ml.job.results.ForecastRequestStats;
import org.elasticsearch.xpack.core.ml.utils.ExceptionsHelper;

import java.io.IOException;
Expand All @@ -33,17 +35,18 @@ public AcknowledgedResponse newResponse() {
return new AcknowledgedResponse();
}

public static class Request extends ActionRequest {
public static class Request extends AcknowledgedRequest<Request> {

private String jobId;
private String forecastId;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Contrary to the other delete requests we currently have, this one deletes results instead of resources. I can imagine the UI allowing users to select multiple forecasts and then delete them in bulk. It would be a pain for the UI to have to perform a request per forecast. I think we should consider allowing this to handle deletion of multiple forecasts. Users can pass a comma separated list of forecast IDs.

private boolean allowNoForecasts = true;

public Request() {
}

public Request(String jobId, String forecastId) {
this.jobId = ExceptionsHelper.requireNonNull(jobId, Job.ID.getPreferredName());
this.forecastId = ExceptionsHelper.requireNonNull(forecastId, ModelSnapshotField.SNAPSHOT_ID.getPreferredName());
this.forecastId = ExceptionsHelper.requireNonNull(forecastId, ForecastRequestStats.FORECAST_ID.getPreferredName());
}

public String getJobId() {
Expand All @@ -54,6 +57,14 @@ public String getForecastId() {
return forecastId;
}

public boolean isAllowNoForecasts() {
return allowNoForecasts;
}

public void setAllowNoForecasts(boolean allowNoForecasts) {
this.allowNoForecasts = allowNoForecasts;
}

@Override
public ActionRequestValidationException validate() {
return null;
Expand All @@ -64,13 +75,15 @@ public void readFrom(StreamInput in) throws IOException {
super.readFrom(in);
jobId = in.readString();
forecastId = in.readString();
allowNoForecasts = in.readBoolean();
}

@Override
public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
out.writeString(jobId);
out.writeString(forecastId);
out.writeBoolean(allowNoForecasts);
}
}

Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/messages/Messages.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,8 @@ public final class Messages {
public static final String REST_JOB_NOT_CLOSED_REVERT = "Can only revert to a model snapshot when the job is closed.";
public static final String REST_NO_SUCH_MODEL_SNAPSHOT = "No model snapshot with id [{0}] exists for job [{1}]";
public static final String REST_START_AFTER_END = "Invalid time range: end time ''{0}'' is earlier than start time ''{1}''.";
public static final String REST_NO_SUCH_FORECAST = "No forecast with id [{0}] exists for job [{1}]";
public static final String REST_BAD_FORECAST_STATE = "Forecast [{0}] for job [{1}] needs to be either FAILED or FINISHED to be deleted";
public static final String REST_NO_SUCH_FORECAST = "No forecast(s) [{0}] exists for job [{1}]";
public static final String REST_CANNOT_DELETE_FORECAST_IN_CURRENT_STATE = "Forecast(s) [{0}] for job [{1}] needs to be either FAILED or FINISHED to be deleted";
public static final String FIELD_CANNOT_BE_NULL = "Field [{0}] cannot be null";

private Messages() {
Expand Down
59 changes: 46 additions & 13 deletions ...ive-multi-node-tests/src/test/java/org/elasticsearch/xpack/ml/integration/ForecastIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.ElasticsearchStatusException;
import org.elasticsearch.action.support.master.AcknowledgedResponse;
import org.elasticsearch.cluster.metadata.MetaData;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.xpack.core.ml.action.DeleteForecastAction;
import org.elasticsearch.xpack.core.ml.job.config.AnalysisConfig;
Expand Down Expand Up @@ -306,25 +307,57 @@ public void testDelete() throws Exception {

postData(job.getId(), data.stream().collect(Collectors.joining()));
flushJob(job.getId(), false);

String forecastIdDefaultDurationDefaultExpiry = forecast(job.getId(), null, null);

String forecastIdDuration1HourNoExpiry = forecast(job.getId(), TimeValue.timeValueHours(1), TimeValue.ZERO);
waitForecastToFinish(job.getId(), forecastIdDefaultDurationDefaultExpiry);
waitForecastToFinish(job.getId(), forecastIdDuration1HourNoExpiry);
closeJob(job.getId());
ForecastRequestStats forecastStats = getForecastStats(job.getId(), forecastIdDefaultDurationDefaultExpiry);
assertNotNull(forecastStats);

DeleteForecastAction.Request request = new DeleteForecastAction.Request(job.getId(), forecastIdDefaultDurationDefaultExpiry);
AcknowledgedResponse response = client().execute(DeleteForecastAction.INSTANCE, request).actionGet();
assertTrue(response.isAcknowledged());
{
ForecastRequestStats forecastStats = getForecastStats(job.getId(), forecastIdDefaultDurationDefaultExpiry);
assertNotNull(forecastStats);
ForecastRequestStats otherStats = getForecastStats(job.getId(), forecastIdDuration1HourNoExpiry);
assertNotNull(otherStats);
}

forecastStats = getForecastStats(job.getId(), forecastIdDefaultDurationDefaultExpiry);
assertNull(forecastStats);
{
DeleteForecastAction.Request request = new DeleteForecastAction.Request(job.getId(),
forecastIdDefaultDurationDefaultExpiry + "," + forecastIdDuration1HourNoExpiry);
AcknowledgedResponse response = client().execute(DeleteForecastAction.INSTANCE, request).actionGet();
assertTrue(response.isAcknowledged());
}

ElasticsearchException e = expectThrows(ElasticsearchException.class,
() -> client().execute(DeleteForecastAction.INSTANCE, request).actionGet());
assertThat(e.getMessage(),
equalTo(String.format("No forecast with id [%s] exists for job [%s]", forecastIdDefaultDurationDefaultExpiry, job.getId())));
{
ForecastRequestStats forecastStats = getForecastStats(job.getId(), forecastIdDefaultDurationDefaultExpiry);
assertNull(forecastStats);
ForecastRequestStats otherStats = getForecastStats(job.getId(), forecastIdDuration1HourNoExpiry);
assertNull(otherStats);
}

{
DeleteForecastAction.Request request = new DeleteForecastAction.Request(job.getId(), "forecast-does-not-exist");
ElasticsearchException e = expectThrows(ElasticsearchException.class,
() -> client().execute(DeleteForecastAction.INSTANCE, request).actionGet());
assertThat(e.getMessage(),
equalTo(String.format("No forecast(s) [forecast-does-not-exist] exists for job [%s]", job.getId())));
}

{
DeleteForecastAction.Request request = new DeleteForecastAction.Request(
"forecasts-delete-with-all-and-allow-no-forecasts", MetaData.ALL);
AcknowledgedResponse response = client().execute(DeleteForecastAction.INSTANCE, request).actionGet();
assertTrue(response.isAcknowledged());
}

{
DeleteForecastAction.Request request = new DeleteForecastAction.Request(
"forecasts-delete-with-all-and-NOT-allow-no-forecasts", MetaData.ALL);
request.setAllowNoForecasts(false);
ElasticsearchException e = expectThrows(ElasticsearchException.class,
() -> client().execute(DeleteForecastAction.INSTANCE, request).actionGet());
assertThat(e.getMessage(),
equalTo(String.format("No forecast(s) [_all] exists for job [forecasts-delete-with-all-and-NOT-allow-no-forecasts]")));
}
}

private void createDataWithLotsOfClientIps(TimeValue bucketSpan, Job.Builder job) throws IOException {
Expand Down
Loading