[ML] Resurrect _xpack/ml routes #77367
Conversation
Related to elastic#51816 / elastic#68905. Adds back the _xpack/ml routes when using rest compatibility for a request.
|
This PR is closely modelled on #73977 - for consistency I've copied the approach from there, including the (near) duplication of spec files. |
|
Pinging @elastic/ml-core (Team:ML) |
| Route.builder(DELETE, BASE_PATH + "_delete_expired_data/{" + Job.ID + "}") | ||
| .replaces(DELETE, PRE_V7_BASE_PATH + "_delete_expired_data/{" + Job.ID + "}", RestApiVersion.V_7).build(), |
There was a problem hiding this comment.
Should we add the pre_v7 path here? It wasn't added. until 7.x
There was a problem hiding this comment.
What are you referring to @benwtrent ? The delete_expired_data action has existed as a REST api earlier than 7.x.
There was a problem hiding this comment.
I can see what it is. Delete expired data can be called with or without a job ID. The option to call it with a job ID was added in 7.x and never had an _xpack fallback. So this is adding BWC in 8.x that didn't exist in 7.x.
However, the problem with enforcing the distinction is that it will cause considerable complication for the tests. At the moment the yamlRestCompatTest setup in 8.x just does a global replace of ml.delete_expired_data with xpack-ml.delete_expired_data. Enforcing the distinction would mean having more complicated test setup. Complexity costs money and I don't think it's worth it. If somebody does start using the newly added deprecated _xpack endpoint in 8.x instead of the correct one then it's likely because they're already using the _xpack everywhere else for ML, are using the REST compatibility layer, and know they have to do a sweep of their code at some point to switch to undeprecated endpoints. So we haven't really caused them any extra work. Anybody who already uses undeprecated endpoints is extremely unlikely to accidentally use a new deprecated _xpack endpoint.
So although pedantically one route should have the compatibility layer and one shouldn't, it's less work for us to just add them both and remove them both together.
| Route.builder(POST, BASE_PATH + "datafeeds/{" + DatafeedConfig.ID + "}/_preview") | ||
| .replaces(POST, PRE_V7_BASE_PATH + "datafeeds/{" + DatafeedConfig.ID + "}/_preview", RestApiVersion.V_7).build(), | ||
| Route.builder(POST, BASE_PATH + "datafeeds/_preview") | ||
| .replaces(POST, PRE_V7_BASE_PATH + "datafeeds/_preview", RestApiVersion.V_7).build() |
There was a problem hiding this comment.
These POST URLs were not added until 7.x. Should we have BWC with pre V7?
There was a problem hiding this comment.
It's the same problem as above. Having some versions of the endpoint support the REST compatibility layer but not others massively complicates the test setup.
| "in a future major version it will be compulsory to use a datafeed", RestApiVersion.V_8 | ||
| ).build() | ||
| Route.builder(POST, BASE_PATH + "anomaly_detectors/{" + Job.ID + "}/_data").deprecated(msg, RestApiVersion.V_8).build(), | ||
| Route.builder(POST, PRE_V7_BASE_PATH + "anomaly_detectors/{" + Job.ID + "}/_data").deprecated(msg, RestApiVersion.V_7).build() |
There was a problem hiding this comment.
Is there a way to do a replaces AND have the additional deprecation message? It seems we are losing that the _xpack URL is old.
There was a problem hiding this comment.
I don't think it's possible to have two deprecation messages for the same endpoint.
It would be possible to have the _ml one say datafeeds will be compulsory in the future and the _xpack one say use the _ml one instead.
But I thought that "don't use this at all" was a more important thing to say than "use something different", especially as the different thing will then tell you not to use it at all.
It's not possible to use just the post data endpoint - you have to use some other ML endpoints too. So users would get the warnings about using _xpack for those other endpoints and hopefully realise that they needed to stop using _xpack everywhere. Also, in 8.x the _xpack version only exists when the REST compatibility layer is used. Presumably the user will test what happens when they stop using the REST compatibility layer before they stop using it in production. So they'd find out that the _xpack version had disappeared at that point, and then it's a simple rename to get back to the deprecation message that the endpoint is going to be removed at some point.
|
@elasticmachine update branch |
|
@elasticmachine update branch |
Related to #51816 / #68905.
Adds back the
_xpack/mlroutes when using rest compatibility for a request.