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 to not require a client factory #7675

Conversation

Projects
None yet
5 participants
@javanna
Copy link
Member

commented Sep 10, 2014

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.

@uboness

View changes

src/main/java/org/elasticsearch/rest/RestController.java Outdated
@@ -44,6 +47,8 @@

public static final String HTTP_JSON_ENABLE = "http.jsonp.enable";

private final Set<String> relevantHeaders = Sets.newCopyOnWriteArraySet();

This comment has been minimized.

Copy link
@uboness

uboness Sep 10, 2014

Contributor

maybe immutable set?

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 10, 2014

Contributor

agreed

This comment has been minimized.

Copy link
@javanna

javanna Sep 10, 2014

Author Member

Not sure I get this...this set is mutable, cause you can potentiallly register relevant headers all the time through registerRelevantHeaders method. Unless we find a way to "seal" the headers after having set them, I think copy on write was better? What am I missing?

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2014

except of the one set / immutable set issue this LGTM much better than before

@spinscale

This comment has been minimized.

Copy link
Member

commented Sep 10, 2014

LGTM, waaaaaay more readable

@javanna

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2014

Pushed a new commit to address comments

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2014

@javanna my last comment was bogus - the builder that I was referring to is the ImmutableMap.Builder, sorry for the confusion. Yet, I think you last commit is a perf issue since now we rebuild a map on each get call which is executed for each request. It will also in the case of adding the same key multiple times just append to an array inside the builder which can consume unneeded memory. Why don't we do this in the setter and rebuild the map each time we add headers?

@s1monw s1monw removed the review label Sep 10, 2014

@javanna

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2014

Why don't we do this in the setter and rebuild the map each time we add headers?

Isn't that what CopyOnWriteArraySet does? I'm missing what the advantage would be for using ImmutableSet here.

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2014

Isn't that what CopyOnWriteArraySet does? I'm missing what the advantage would be for using ImmutableSet here.

sorry that I am not expressing myself clear enough. The CopyOnWriteArraySet does copy on write but we want an immutable set so if you return the instance from relevantHeaders() CopyOnWriteArraySet will be mutable outside of the RestController, makes sense?

@javanna

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2014

we want an immutable set so if you return the instance from relevantHeaders() CopyOnWriteArraySet will be mutable outside of the RestController, makes sense?

Makes sense, I had missed the public getter that exposed the set. I pushed a new commit that should fix this.

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
@uboness

View changes

src/main/java/org/elasticsearch/rest/RestController.java Outdated
* its corresponding {@link org.elasticsearch.transport.TransportRequest}(s).
* By default no headers get copied but it is possible to extend this behaviour via plugins by calling {@link #registerRelevantHeaders(String...)}.
*/
public Set<String> relevantHeaders() {

This comment has been minimized.

Copy link
@uboness

uboness Sep 11, 2014

Contributor

I'd return an Immutable set... it enforce compilation failure when used inappropriately (if the user tries to mutate the set)

This comment has been minimized.

Copy link
@javanna

javanna Sep 11, 2014

Author Member

makes sense will change that

@javanna javanna force-pushed the javanna:enhancement/relevant_headers_rest_controller branch to 200f845 Sep 11, 2014

@uboness

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2014

LGTM

@javanna javanna closed this in 4ab268b Sep 11, 2014

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 to not require a client factory Refactor copy headers mechanism to not require a client factory 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.