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

Deprecate xpack and typed endpoints for graph explore api #74230

Merged
merged 4 commits into from
Jun 29, 2021
Merged

Deprecate xpack and typed endpoints for graph explore api #74230

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
601e54d
deprecate xpack and typed endpoints for graph explore api
pgomulka Jun 17, 2021
1fd1987
Merge remote-tracking branch 'upstream/7.x' into depreacte_graph_type…
pgomulka Jun 17, 2021
c84e855
Merge remote-tracking branch 'upstream/7.x' into depreacte_graph_type…
pgomulka Jun 23, 2021
ae65115
cover all methods/paths
pgomulka Jun 23, 2021
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
3 changes: 2 additions & 1 deletion server/src/main/java/org/elasticsearch/rest/RestController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() ?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

OOF! Good catch. This is a mistake IMO; we should do one of the following things:

  1. Remove ThreadContext from the request dispatcher API and always use the one from the node client
  2. Leave the API alone and pass the ThreadContext from the dispatcher to all the other methods
  3. 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

final ThreadContext threadContext = client.threadPool().getThreadContext();
if (handler.allowSystemIndexAccessByDefault() == false) {
// The ELASTIC_PRODUCT_ORIGIN_HTTP_HEADER indicates that the request is coming from an Elastic product and
Expand Down Expand Up @@ -352,6 +352,7 @@ private void tryAllHandlers(final RestRequest request, final RestChannel channel
return;
}
} else {
//TODO not passing thread context?
dispatchRequest(request, channel, handler);
return;
}
Expand Down
5 changes: 4 additions & 1 deletion test/framework/src/main/java/org/elasticsearch/test/rest/RestActionTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

no need to create this anymore

Suggested change
ThreadContext threadContext = new ThreadContext(Settings.EMPTY);

controller.dispatchRequest(request, channel, threadContext);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


try (ThreadContext.StoredContext ignore = verifyingClient.threadPool().getThreadContext().stashContext()) {
controller.dispatchRequest(request, channel, threadContext);
}
}

/**
Expand Down
12 changes: 9 additions & 3 deletions ...plugin/graph/src/main/java/org/elasticsearch/xpack/graph/rest/action/RestGraphAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,16 @@ public List<Route> routes() {
.replaces(GET, "/{index}" + URI_BASE + "/graph/_explore", RestApiVersion.V_7).build(),
Route.builder(POST, "/{index}/_graph/explore")
.replaces(POST, "/{index}" + URI_BASE + "/graph/_explore", RestApiVersion.V_7).build(),

Route.builder(GET, "/{index}/{type}" + URI_BASE + "/graph/_explore")
.deprecated(TYPES_DEPRECATION_MESSAGE, RestApiVersion.V_7).build(),
Route.builder(POST, "/{index}/{type}" + URI_BASE + "/graph/_explore")
.deprecated(TYPES_DEPRECATION_MESSAGE, RestApiVersion.V_7).build(),

Route.builder(GET, "/{index}/{type}/_graph/explore")
.replaces(GET, "/{index}/{type}" + URI_BASE + "/graph/_explore", RestApiVersion.V_7).build(),
Route.builder(POST, "/{index}/{type}_graph/explore")
.replaces(POST, "/{index}/{type}" + URI_BASE + "/graph/_explore", RestApiVersion.V_7).build()
.deprecated(TYPES_DEPRECATION_MESSAGE, RestApiVersion.V_7).build(),
Route.builder(POST, "/{index}/{type}/_graph/explore")
.deprecated(TYPES_DEPRECATION_MESSAGE, RestApiVersion.V_7).build()
));
}

Expand Down
51 changes: 32 additions & 19 deletions ...n/graph/src/test/java/org/elasticsearch/xpack/graph/rest/action/RestGraphActionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import org.junit.Before;

import java.util.HashMap;
import java.util.Map;

public class RestGraphActionTests extends RestActionTestCase {

Expand All @@ -26,25 +27,37 @@ public void setUpAction() {
}

public void testTypeInPath() {
RestRequest request = new FakeRestRequest.Builder(xContentRegistry())
.withMethod(RestRequest.Method.GET)
.withPath("/some_index/some_type/_graph/explore")
.withContent(new BytesArray("{}"), XContentType.JSON)
.build();
// We're not actually testing anything to do with the client, but need to set this so it doesn't fail the test for being unset.
verifyingClient.setExecuteVerifier(
(arg1, arg2) -> new GraphExploreResponse(
0,
false,
new ShardOperationFailedException[0],
new HashMap<>(),
new HashMap<>(),
false
)
);

dispatchRequest(request);
assertWarnings(RestGraphAction.TYPES_DEPRECATION_MESSAGE);
for(Map.Entry<RestRequest.Method,String> methodAndPath :
org.elasticsearch.core.Map.of(
RestRequest.Method.GET, "/some_index/some_type/_graph/explore",
RestRequest.Method.POST, "/some_index/some_type/_graph/explore",
RestRequest.Method.GET, "/some_index/some_type/_xpack/graph/_explore",
RestRequest.Method.POST, "/some_index/some_type/_xpack/graph/_explore"
).entrySet()) {

RestRequest request = new FakeRestRequest.Builder(xContentRegistry())
.withMethod(methodAndPath.getKey())
.withPath(methodAndPath.getValue())
.withContent(new BytesArray("{}"), XContentType.JSON)
.build();
// We're not actually testing anything to do with the client,
// but need to set this so it doesn't fail the test for being unset.
verifyingClient.setExecuteVerifier(
(arg1, arg2) -> new GraphExploreResponse(
0,
false,
new ShardOperationFailedException[0],
new HashMap<>(),
new HashMap<>(),
false
)
);

dispatchRequest(request);
assertWarnings(RestGraphAction.TYPES_DEPRECATION_MESSAGE);

}

}

}