Deprecate xpack and typed endpoints for graph explore api #74230
Conversation
| @@ -253,7 +253,7 @@ private void dispatchRequest(RestRequest request, RestChannel channel, RestHandl | |||
| if (handler.allowsUnsafeBuffers() == false) { | |||
| request.ensureSafeBuffers(); | |||
| } | |||
|
|
|||
| //TODO why we are using client.threadPool().getThreadContext() ? | |||
There was a problem hiding this comment.
I wonder if we should not be using the thread context passed over via AbstractHttpServerTransport#dispatchRequest
The thread pool is first created in node and is the used in both RestController and AbstracthttpServerTransport
There was a problem hiding this comment.
OOF! Good catch. This is a mistake IMO; we should do one of the following things:
- Remove ThreadContext from the request dispatcher API and always use the one from the node client
- Leave the API alone and pass the ThreadContext from the dispatcher to all the other methods
- Add an assertion in dispatch*Request methods that assert the passed threadcontext is the same as that on the node client
This mainly is just an issue for testing but it is tricky and would be nice if we just had one way to access the value
| @@ -66,7 +66,10 @@ protected RestController controller() { | |||
| protected void dispatchRequest(RestRequest request) { | |||
| FakeRestChannel channel = new FakeRestChannel(request, false, 1); | |||
| ThreadContext threadContext = new ThreadContext(Settings.EMPTY); | |||
| controller.dispatchRequest(request, channel, threadContext); | |||
There was a problem hiding this comment.
this in fact was not really chaning the context used in RestController - not in all places.
In same places the thread context fetched from thread pool was used
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| @@ -253,7 +253,7 @@ private void dispatchRequest(RestRequest request, RestChannel channel, RestHandl | |||
| if (handler.allowsUnsafeBuffers() == false) { | |||
| request.ensureSafeBuffers(); | |||
| } | |||
|
|
|||
| //TODO why we are using client.threadPool().getThreadContext() ? | |||
There was a problem hiding this comment.
OOF! Good catch. This is a mistake IMO; we should do one of the following things:
- Remove ThreadContext from the request dispatcher API and always use the one from the node client
- Leave the API alone and pass the ThreadContext from the dispatcher to all the other methods
- Add an assertion in dispatch*Request methods that assert the passed threadcontext is the same as that on the node client
This mainly is just an issue for testing but it is tricky and would be nice if we just had one way to access the value
| @@ -66,7 +66,10 @@ protected RestController controller() { | |||
| protected void dispatchRequest(RestRequest request) { | |||
| FakeRestChannel channel = new FakeRestChannel(request, false, 1); | |||
| ThreadContext threadContext = new ThreadContext(Settings.EMPTY); | |||
There was a problem hiding this comment.
no need to create this anymore
| ThreadContext threadContext = new ThreadContext(Settings.EMPTY); |
Previously the
/{index}/{type}/_xpack/graph/_exploreapi was indicating to use/{index}/{type}/_graph/explorewhich is also deprecated.This commit deprecates all typed endpoints