Copy the headers from REST requests to the corresponding TransportRequest(s) #6513

javanna · 2014-06-16T13:07:28Z

Introduced the use of the FilterClient in all of the REST actions, which delegates all of the operations to the internal Client, but makes sure that the headers are properly copied if needed from REST requests to TransportRequest(s) when it comes to executing them. Added new abstract handleRequest method to BaseRestHandler with additional Client argument and made private the client instance member (was protected before) to force the use of the client received as argument. The list of headers to be copied over is by default empty but can be extended via plugins.

Replaces #6464 as it provides a better and safer way to copy headers from REST layer to transport layer.

uboness · 2014-06-16T15:11:54Z

src/main/java/org/elasticsearch/rest/HeadersCopyClient.java

+  * but makes sure that the REST headers are copied over from {@link org.elasticsearch.rest.RestRequest} to the
+  * corresponding {@link org.elasticsearch.action.ActionRequest}(s)
+  */
+public class HeadersCopyClient extends AbstractClient {


I think this one should be an inner class in BaseRestHandler

there should probably be a common client for all client types... and then it'll just work with whatever client

even after #6517 this is not so easy to achieve keeping type-safety at the same time, because the Client generic type which can either be Client, ClusterAdminClient or IndicesAdminClient. I'm opting for duplicating the execute methods, it's still less LOC than trying to generalize this so that we have a single pair of execute methods.

…equests to the corresponding TransportRequest(s) Introduced the use of a FilterClient in all of the REST actions, which delegates all of the operations to the internal Client, but makes sure that the headers are properly copied from REST requests to TransportRequest(s) when it comes to executing them. Closes elastic#6513

javanna · 2014-06-17T15:06:48Z

I updated the PR after #6517 was committed, ready for review again...

uboness · 2014-06-17T15:53:46Z

src/main/java/org/elasticsearch/rest/BaseRestHandler.java

+        }
+
+        @Override
+        public IndicesAdminClient indices() {


Can we keep the refs to these internal clients

makes sense, just pushed a new commit that addresses this

s1monw · 2014-06-18T09:35:12Z

I think this looks awesome. I am hesitating with the BW break here, IMO it would be nicer to deprecate the BaseRestHandler#client and add a method BaseRestHandler#client(RestRequest) instead of adding a new abstract method. We can then make the member private in master and drop the deprecation?

kimchy · 2014-06-18T09:50:41Z

does it make sense when we copy over the headers, to not copy over common known HTTP headers that are not really used, like Content-Type, Content-Length, ...

uboness · 2014-06-18T09:56:02Z

@s1monw I think it's actually better to pass in the client as an arg to handleRequest as it "encourage" handler implementations to use the client. Nothing really prevents handler impl. from injecting transport actions directly and bypass the client all together. The only bwc breaking we have here, relates to plugins, and we're breaking bwc for plugins almost every other release (e.g. when we upgrade lucene version).. I think this change is important enough to justify it.

@kimchy yea, it makes sense +1

s1monw · 2014-06-18T11:13:56Z

fair enough... LGTM

… via plugins

javanna · 2014-06-18T13:50:25Z

Pushed a new commit containing a static list of REST headers that need to be copied over to the transport.

uboness · 2014-06-18T13:53:10Z

src/main/java/org/elasticsearch/rest/BaseRestHandler.java

 */
 public abstract class BaseRestHandler extends AbstractComponent implements RestHandler {

-    protected final Client client;
+    private static final Set<String> usefulHeaders = new HashSet<>();


this should probably be an array

updated tests too to have useful headers that are not part of the rest request

…c addUsefulHeader works (from multiple threads as well)

javanna · 2014-06-18T15:24:34Z

Pushed a couple of new commits that address @uboness feedback, it should be close.

uboness · 2014-06-19T15:57:32Z

LGTM

…s to the corresponding TransportRequest(s) Introduced the use of the FilterClient in all of the REST actions, which delegates all of the operations to the internal Client, but makes sure that the headers are properly copied if needed from REST requests to TransportRequest(s) when it comes to executing them. Added new abstract handleRequest method to BaseRestHandler with additional Client argument and made private the client instance member (was protected before) to force the use of the client received as argument. The list of headers to be copied over is by default empty but can be extended via plugins. Closes #6513

javanna · 2014-06-19T17:31:21Z

As a side note, this change is breaking for plugins that add their own REST handlers by extending BaseRestHandler. They can easily adapt just by adding an additional Client argument to their existing handleRequest method and use that Client received as argument to execute the request instead of this.client.

…nsport requests The useful headers are now stored into a `Set` instead of an array so we can easily deduplicate them. A set is also returned instead of an array by the `usefulHeaders` static getter. Relates to elastic#6513 Closes elastic#7590

…nsport requests The useful headers are now stored into a `Set` instead of an array so we can easily deduplicate them. A set is also returned instead of an array by the `usefulHeaders` static getter. Relates to #6513 Closes #7590

The functionality of copying headers in the REST layer (from REST requests to transport requests) remains the same. Made it a bit nicer by introducing a ClientFactory component that is a singleton and allows to register useful headers without requiring static methods. Plugins just have to inject the ClientFactory now, and call its `addUsefulHeaders` method that is not static anymore. Relates to elastic#6513 Closes elastic#7594

The functionality of copying headers in the REST layer (from REST requests to transport requests) remains the same. Made it a bit nicer by introducing a RestClientFactory component that is a singleton and allows to register useful headers without requiring static methods. Plugins just have to inject the RestClientFactory now, and call its `addRelevantHeaders` method that is not static anymore. Relates to #6513 Closes #7594

javanna added enhancement labels Jun 16, 2014

javanna self-assigned this Jun 16, 2014

javanna added the review label Jun 16, 2014

This was referenced Jun 16, 2014

Transport: copied over REST headers from RestRequest to TransportRequest #6464

Closed

Remove unnecessary intermediate interfaces #6517

Merged

uboness reviewed Jun 16, 2014
View reviewed changes

uboness reviewed Jun 17, 2014
View reviewed changes

reused same instance of admin clients

4d1f444

s1monw removed the review label Jun 18, 2014

Added static list of headers to copy, empty by default but extensible…

a2225f2

… via plugins

javanna changed the title ~~REST api: made sure that the REST headers are copied over from REST requests to the corresponding TransportRequest(s)~~ REST api: made it possible to copy the REST headers over from REST requests to the corresponding TransportRequest(s) Jun 18, 2014

uboness reviewed Jun 18, 2014
View reviewed changes

javanna added 3 commits June 18, 2014 16:18

changed the way the headers are copied, iterate over useful headers only

8480a05

updated tests too to have useful headers that are not part of the rest request

replaced Set<String> with String[], added test to make sure the stati…

b62fcdd

…c addUsefulHeader works (from multiple threads as well)

removed needless log lines

4205021

made usefulHeaders() method package private (only used in test)

7d4fa4b

javanna closed this in 12fd6ce Jun 19, 2014

clintongormley changed the title ~~REST api: made it possible to copy the REST headers over from REST requests to the corresponding TransportRequest(s)~~ REST API: Copy the headers from REST requests to the corresponding TransportRequest(s) Jul 16, 2014

javanna mentioned this pull request Sep 4, 2014

Refactor copy headers mechanism in REST API #7594

Closed

clintongormley added the :Internal label Jun 7, 2015

clintongormley changed the title ~~REST API: Copy the headers from REST requests to the corresponding TransportRequest(s)~~ Copy the headers from REST requests to the corresponding TransportRequest(s) Jun 7, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy the headers from REST requests to the corresponding TransportRequest(s) #6513

Copy the headers from REST requests to the corresponding TransportRequest(s) #6513

javanna commented Jun 16, 2014

uboness Jun 16, 2014

uboness Jun 16, 2014

javanna Jun 17, 2014

javanna commented Jun 17, 2014

uboness Jun 17, 2014

javanna Jun 17, 2014

s1monw commented Jun 18, 2014

kimchy commented Jun 18, 2014

uboness commented Jun 18, 2014

s1monw commented Jun 18, 2014

javanna commented Jun 18, 2014

uboness Jun 18, 2014

javanna commented Jun 18, 2014

uboness commented Jun 19, 2014

javanna commented Jun 19, 2014

Navigation Menu

Copy the headers from REST requests to the corresponding TransportRequest(s) #6513

Copy the headers from REST requests to the corresponding TransportRequest(s) #6513

Conversation

javanna commented Jun 16, 2014

uboness Jun 16, 2014

Choose a reason for hiding this comment

uboness Jun 16, 2014

Choose a reason for hiding this comment

javanna Jun 17, 2014

Choose a reason for hiding this comment

javanna commented Jun 17, 2014

uboness Jun 17, 2014

Choose a reason for hiding this comment

javanna Jun 17, 2014

Choose a reason for hiding this comment

s1monw commented Jun 18, 2014

kimchy commented Jun 18, 2014

uboness commented Jun 18, 2014

s1monw commented Jun 18, 2014

javanna commented Jun 18, 2014

uboness Jun 18, 2014

Choose a reason for hiding this comment

javanna commented Jun 18, 2014

uboness commented Jun 19, 2014

javanna commented Jun 19, 2014