Added an option to add arbitrary headers to the client requests #7127
Conversation
| // TODO this is a really shitty way to test it, we need to figure out a way to test all the client methods | ||
| // without specifying each one (reflection doesn't as each action needs its own special settings, without | ||
| // them, request validation will fail before the test is executed. (one option is to enable disabling the | ||
| // validation in the settings??? - ugly and conceptually wrong) |
There was a problem hiding this comment.
yeah...been there...done that... :) even if you disable requests validation you'll run into problems when serializing the request (writeTo method). Can't you use a fake request so that validation and serialization are not a problem, and then call execute and check that requests get set? Not sure it is really needed to check every single request at this point?
I'd love to see a unit test around this though, I don't think this necessarily requires a running node, you could just create your own mock transport service that checks that each request contains the headers (you don't have to send the request for real). You could make it work without even requiring a transport client but by testing InternalTransportClient as I did here: https://github.com/elasticsearch/elasticsearch/blob/master/src/test/java/org/elasticsearch/client/transport/InternalTransportClientTests.java .
Also, I'm a bit hesitant on the changes to the test infrastructure that are needed for this single test. I'd rather prefer to have a manually created transport client here with the settings, or not even require a complete transport client if you go for the unit test.
There was a problem hiding this comment.
yeah...been there...done that... :) even if you disable requests validation you'll run into problems when serializing the request (writeTo method). Can't you use a fake request so that validation and serialization are not a problem, and then call execute and check that requests get set? Not sure it is really needed to check every single request at this point?
actually, without validation reflection approach would work (the mock transport shortcuts execution before requests are serialized). But enabling an option to disable validation, just for testing, feels wrong (there's no scenario anyone should be able to do this outside of testing)
I'd love to see a unit test around this though, I don't think this necessarily requires a running node, you could just create your own mock transport service that checks that each request contains the headers (you don't have to send the request for real). You could make it work without even requiring a transport client but by testing InternalTransportClient as I did here:
fair enough... I'll see if I can change the test to not depend on the cluster at all
Also, I'm a bit hesitant on the changes to the test infrastructure that are needed for this single test. I'd rather prefer to have a manually created transport client here with the settings, or not even require a complete transport client if you go for the unit test.
This is actually a good extension for the testing infra. it should be possible to customize the configuration of transport clients (right now it's not possible). But I agree that if this PR won't depend on the cluster infra, it's better to create a separate PR for it
There was a problem hiding this comment.
Regarding the testing infra extension, the one thing I don't get is where we would need to use it. If it's needed in some tests (other than this) I'm +1 on adding it, otherwise I don't quite see the point. Am I missing something?
There was a problem hiding this comment.
well... the point is that configuring nodes and configuring transport clients are two different things. Right now, the transport client configuration is randomized internally without the ability to modify it.. you might want to have your own custom settings for the client. The internal randomization is still there, but at least have the ability to override/force certain settings.
There was a problem hiding this comment.
I understand the difference between the two...just saying that we are adding an extension point that is currently not used. But if we plan on using it in the future, thats' great. Just reluctant to add complexity when we don't need it.
|
Left some comments, mainly around testing, which is indeed quite a challenge here but I think we can work on it |
|
updated... change the test to no depend on the internal cluster infrastructure, but there's till room for improvement |
| headers = resolveHeaders(settings); | ||
| } | ||
|
|
||
| public void applyTo(ActionRequest request) { |
There was a problem hiding this comment.
I like applyTo better than set! Out of curiosity, any specific reason why you don't want to move this logic to the ActionRequest?
There was a problem hiding this comment.
not sure where you want to put it... the headers are injected on the clients (it's a client only feature)
|
Left a couple comments, again around testing , but this looks already much better ;) |
|
@javanna updated... have a look |
| */ | ||
| public class AbstractClientHeadersTests extends ElasticsearchTestCase { | ||
|
|
||
| protected static final Settings HEADER_SETTGINS = ImmutableSettings.builder() |
There was a problem hiding this comment.
s/HEADER_SETTGINS/HEADER_SETTINGS
| private Actions(Settings settings, ThreadPool threadPool, Class[] actionClasses) { | ||
| this.settings = settings; | ||
| this.threadPool = threadPool; | ||
| for (int i = 0; i < actionClasses.length; i++) { |
There was a problem hiding this comment.
this could be a for each loop instead
|
Getting closer, left some comments, would love it if somebody else could look at it, especially the tests, @jpountz maybe? |
| @Override | ||
| public void onFailure(Throwable t) { | ||
| Throwable e = unwrap(t, InternalException.class); | ||
| assertThat("exected action [" + action + "] to throw an internal exception", e, notNullValue()); |
There was a problem hiding this comment.
sorry, I can't help it... typo s/exected/expected
There was a problem hiding this comment.
I'm afraid it's me... not you ;)
The headers are key/value pairs defined in the settings under the `request.headers` namespace.
|
LGTM |
|
merged by 5a2bf32 |
clintongormley
changed the title
clintongormley
changed the title
The headers are key/value pairs defined in the settings under the
request.headersnamespace.Also changed test infrastructure's so it'd be possible to define the additional settings for transport clients.