[ML][Data Frame] adding force delete #44590
Conversation
|
Pinging @elastic/ml-core |
| @@ -141,6 +141,12 @@ teardown: | |||
| "pivot": { | |||
| "group_by": { "airline": {"terms": {"field": "airline"}}}, | |||
| "aggs": {"avg_response": {"avg": {"field": "responsetime"}}} | |||
| }, | |||
There was a problem hiding this comment.
I changed this config on purpose. I noticed while writing the yaml test for this PR, that this particular test was saying that its transform was continuous, but it actually wasn't. So, I just fixed it.
| `force`:: | ||
| (Optional, boolean) When `true`, the {dataframe-transform} is deleted regardless of its | ||
| current state. The default value is `false`, meaning that the {dataframe-transform} must be | ||
| `stopped`. |
There was a problem hiding this comment.
Maybe extend the last sentence to: "...must be stopped before it can be deleted."
| if (pTasksMeta != null && pTasksMeta.getTask(request.getId()) != null) { | ||
| ActionListener<AcknowledgedResponse> listener) { | ||
| final PersistentTasksCustomMetaData pTasksMeta = state.getMetaData().custom(PersistentTasksCustomMetaData.TYPE); | ||
| if (pTasksMeta != null && pTasksMeta.getTask(request.getId()) != null && request.isForce() == false) { | ||
| listener.onFailure(new ElasticsearchStatusException("Cannot delete data frame [" + request.getId() + | ||
| "] as the task is running. Stop the task first", RestStatus.CONFLICT)); |
There was a problem hiding this comment.
Adding "return;" after this line and getting rid of the "else" branch in line 72 would benefit readability IMO.
| listener.onResponse(new AcknowledgedResponse(r)); | ||
| }, | ||
| listener::onFailure)); | ||
| ActionListener<Void> stopTransformActionListener = ActionListener.wrap( |
There was a problem hiding this comment.
[optional] IMO more readable code could be achieved by:
CheckedRunnable deleteTransform = () -> transformsConfigManager.deleteTransform(..., ...);
if (pTasksMeta != null && pTasksMeta.getTask(request.getId()) != null) {
executeAsyncWithOrigin(
...,
ActionListener.wrap(r -> deleteTransform.run(), listener::onFailure));
} else {
deleteTransform.run();
}
I see 2 advantages here:
- variable "deleteTransform" clearly describes what it does and is not tied to some other action's listener.
- no need to pass unused parameters to it
WDYT?
There was a problem hiding this comment.
I prefer to try and keep callbacks uniformed. Our callback stacks can get very complicated and to me its just easier to reason about if they are ActionListener + and always within the flow (if possible)
* [ML][Data Frame] adding force delete * Update delete-transform.asciidoc
|
Documentation LGTM, thanks! |
Adds an optional boolean flag of
forcetoDELETE.force=trueallows for a data frame transform to be deleted no matter what its running state. Essentially, it adds a branch onif(forced)that will stop the data frame being deleted if it is running. Then, once it is stopped, it will be deleted.closes #43961