[ML] Data Frame HLRC start & stop APIs #40154
Conversation
davidkyle
added
:Core/Features/Java High Level REST Client
v8.0.0
v7.2.0
:ml/Transform
|
Pinging @elastic/es-core-features |
|
Pinging @elastic/ml-core |
davidkyle
force-pushed
the
df-hlrc-stop-api
branch
from
3ac70af to
679a44d
Compare
|
I pushed a change to add a teardown stopping and deleting transforms to the docs tests. Also I added parsing the @benwtrent can you look over the latest 2 commits please |
benwtrent
left a comment
benwtrent
left a comment
There was a problem hiding this comment.
Minor things, looks good though .
| String id = restRequest.param(DataFrameField.ID.getPreferredName()); | ||
| StartDataFrameTransformAction.Request request = new StartDataFrameTransformAction.Request(id); | ||
|
|
||
| if (restRequest.hasParam(DataFrameField.TIMEOUT.getPreferredName())) { |
There was a problem hiding this comment.
I think this hasParam check is redundant, though it is not hurting anything.
| "timeout": { | ||
| "type": "time", | ||
| "required": false, | ||
| "description": "Controls the time to wait for the transform to start" |
There was a problem hiding this comment.
would be nice to know the default value.
There was a problem hiding this comment.
I'm having second thoughts about this timeout parameter. Get Stats, Preview and Put could all take a timeout parameter as ActionRequests or BaseTasksRequests. Should we actually expose it? Is it helpful to offer this option or confusing? I think it makes sense for Stop and Start (start datafeed and open job both have a timeout parameter). I don't know anymore I'm over-thinking it.
There was a problem hiding this comment.
GET stats _preview and PUT should not really have a timeout option (IMO).
_start could be take a long time as it needs to wait for a Node to start executing the task, thus a timeout sort of makes sense.
Addendum, if we include timeout we may also want to include a wait_for_completion or a sync/async option, but that could be another PR and discussion.
There was a problem hiding this comment.
I agree that _start and _stop should take a timeout parameter and the others shouldn't. For _start and _stop there will be occasions when the caller wants to know when the state change is complete before they move on to the next operation, yet they probably don't want to wait forever if something goes wrong. Our integration tests are a good example of this need but any user trying to automate an end-to-end process that involves data frames will be in a similar situation. Having a timeout on the _start and _stop endpoints is also consistent with what we do for datafeeds and anomaly detectors.
There was a problem hiding this comment.
Makes sense. Thanks for the 2nd review @benwtrent
colings86
added
>enhancement
and removed
:Core/Features/Java High Level REST Client
labels
6 participants
On the server side both these APIs inherit
BaseTasksResponsewhich returns node failures etc. This PR adds aAcknowledgedTasksResponsefor parsing the response.The
TaskOperationFailureclass was added in #29546 and the decision was made there not to implementequals&hashcodeas the serialisation of ElasticsearchExceptions changes them wrapping the error in an outer exception. This made writing the unit tests a little bit more complicated as a custom equals method was required.