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.

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

Refactor copy headers mechanism in REST API #7594

Closed

Conversation

Projects
None yet
3 participants
@javanna
Copy link
Member

commented Sep 4, 2014

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 #6513

@javanna javanna self-assigned this Sep 4, 2014

@javanna javanna force-pushed the javanna:enhancement/rest_client_factory branch Sep 4, 2014

@s1monw

View changes

src/main/java/org/elasticsearch/rest/ClientFactory.java Outdated
* Returns a proper {@link Client client} given the provided {@link org.elasticsearch.rest.RestRequest}
*/
public Client client(RestRequest restRequest) {
return usefulHeaders.size() == 0 ? client : new HeadersCopyClient(client, restRequest, usefulHeaders);

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 5, 2014

Contributor

maybe use isEmpty()?

@s1monw

View changes

src/main/java/org/elasticsearch/rest/ClientFactory.java Outdated
* Makes it possible to register useful headers that will be copied over from REST requests
* to corresponding transport requests at execution time.
*/
public final class ClientFactory {

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 5, 2014

Contributor

I don't necessarily like the name here really - maybe we can name this RestClient instead?

This comment has been minimized.

Copy link
@javanna

javanna Sep 9, 2014

Author Member

Agreed the name isn't great

@s1monw

View changes

src/main/java/org/elasticsearch/rest/ClientFactory.java Outdated
*
* By default no headers get copied but it is possible to extend this behaviour via plugins by calling this method.
*/
public void addUsefulHeaders(String... headers) {

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 5, 2014

Contributor

hmm maybe name this addRelevantHeaders

This comment has been minimized.

Copy link
@javanna

javanna Sep 9, 2014

Author Member

Why not? Just note that this name was previously introduced, but we can still change it since it's part of 1.4 which hasn't been released yet.

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2014

I left some minor comments

@s1monw s1monw removed the review label Sep 5, 2014

@clintongormley clintongormley changed the title REST API: refactor copy headers mechanism Internal: Refactor copy headers mechanism in REST API Sep 8, 2014

javanna added some commits Sep 4, 2014

Internal: refactor copy headers mechanism
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 #6513
Closes #7594

@javanna javanna force-pushed the javanna:enhancement/rest_client_factory branch to 2d399ef Sep 9, 2014

@javanna javanna added the review label Sep 9, 2014

@javanna

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2014

I pushed a new commit that addresses the naming feedback. I also rebased to adapt the code to #7610 that was pushed in the meantime.

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2014

LGTM

@s1monw s1monw removed the review label Sep 10, 2014

javanna added a commit that referenced this pull request Sep 10, 2014

Internal: refactor copy headers mechanism
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 javanna closed this in 5bea31c Sep 10, 2014

javanna added a commit to javanna/elasticsearch that referenced this pull request Sep 11, 2014

Internal: refactor copy headers mechanism to not require a client fac…
…tory

With elastic#7594 we replaced the static `BaseRestHandler#addUsefulHeaders` by introducing the `RestClientFactory` that can be injected and used to register the relevant headers. To simplify things, we can now register relevant headers through the `RestController` and remove the `RestClientFactory` that was just introduced.

Closes elastic#7675

javanna added a commit that referenced this pull request Sep 11, 2014

Internal: refactor copy headers mechanism to not require a client fac…
…tory

With #7594 we replaced the static `BaseRestHandler#addUsefulHeaders` by introducing the `RestClientFactory` that can be injected and used to register the relevant headers. To simplify things, we can now register relevant headers through the `RestController` and remove the `RestClientFactory` that was just introduced.

Closes #7675

javanna added a commit that referenced this pull request Sep 11, 2014

Internal: refactor copy headers mechanism to not require a client fac…
…tory

With #7594 we replaced the static `BaseRestHandler#addUsefulHeaders` by introducing the `RestClientFactory` that can be injected and used to register the relevant headers. To simplify things, we can now register relevant headers through the `RestController` and remove the `RestClientFactory` that was just introduced.

Closes #7675

@clintongormley clintongormley changed the title Internal: Refactor copy headers mechanism in REST API Refactor copy headers mechanism in REST API Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.