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

Introduced a transient context to the rest request #7610

Merged
merged 1 commit into from Sep 5, 2014

Conversation

Projects
None yet
5 participants
@uboness
Copy link
Contributor

uboness commented Sep 5, 2014

Similar to the one in TransportMessage. Added the ContextHolder base class where both TransportMessage and RestRequest derive from

Now next to the known headers, the context is always copied over from the rest request to the transport request (when the injected client is used)

Introduced a transient context to the rest request
Similar to the one in `TransportMessage`. Added the `ContextHolder` base class where both `TransportMessage` and `RestRequest` derive from

Now next to the known headers, the context is always copied over from the rest request to the transport request (when the injected client is used)

@uboness uboness force-pushed the uboness:enhance/rest_request_ctx branch to 5df9c04 Sep 5, 2014

@kimchy

This comment has been minimized.

Copy link
Member

kimchy commented Sep 5, 2014

LGTM

@uboness uboness merged commit 5df9c04 into elastic:master Sep 5, 2014

@clintongormley clintongormley changed the title Introduced a transient context to the rest request Internal: Introduced a transient context to the rest request Sep 8, 2014

@javanna

This comment has been minimized.

@uboness are we sure that we want to copy the request context only if there are usefulHeaders registered? I think this condition might be wrong now that we copy the context too?

This comment has been minimized.

Copy link
Contributor Author

uboness replied Sep 9, 2014

aaaaiiiiiii... good catch!!!!! we always want to copy the context regardless of the headers

This comment has been minimized.

Copy link
Member

javanna replied Sep 9, 2014

Ok, downside of this is that we will always need to wrap the client, while previously we would do it only if a plugin registered useful headers, empty by default. Do we want to do anything about this? Maybe allow to disable copying the context?

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

Internal: make sure that the request context is always copied from RE…
…ST to transport layer

Also renamed HeadersCopyClientTests since it tests a class that was renamed and removed randomization around client wrapper, as it now needs to be weapped all the time to copy the context, doesn't depend on useful headers that have been registered anymore.

Relates to #7610

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

Internal: make sure that the request context is always copied from RE…
…ST to transport layer

Also renamed HeadersCopyClientTests since it tests a class that was renamed and removed randomization around client wrapper, as it now needs to be weapped all the time to copy the context, doesn't depend on useful headers that have been registered anymore.

Relates to #7610

@jpountz jpountz removed the review label Oct 21, 2014

@clintongormley clintongormley changed the title Internal: Introduced a transient context to the rest request Introduced a transient context to the rest request 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.