The callback replaces the ability to fully replace the http client instance. By doing that, one used to lose any default that the RestClient had set for the underlying http client. Given that you'd usually override one or two things only, like a couple of timeout values, the ssl factory or the default credentials providers, it is not uder friendly if by doing that users end up replacing the whole http client instance and lose any default set by us.
Users wanting to send a request by providing only its method and endpoint, effectively the only two required arguments, shouldn't need to pass in an empty map and a null entity for the body. While at it we can also add a variant to send requests by specifying only method, endpoint and params, but not body. Headers remain a vararg as last argument, so they can always optionally be provided.
Closes#19312
The assumption is HostsSniffer is that all of the arguments have been properly provided and validated through HostsSniffer.Builder, except they weren't, as the scheme didn't have a default value and when not set would cause NPEs down the road. Improved tests to catch this also.
Some Rest tests use the Sun HTTP server which has lingering threads after shutdown. Similar to ESTestCase, this adds an
option to wait 5 seconds for these threads to terminate.
:client ---------> :client:rest
:client-sniffer -> :client:sniffer
:client-test ----> :client:test
This lines the client up with how we do things like modules and
plugins.
The lucene-test dependency caused issues with IDEs as they would always load the lucene 5 jar although they shouldn't have, which caused jarhell in es core tests.
If we depend directly on randomized runner we don't have this problem. It is luckily still compatible with java 1.7. This requires though adding a thin module that includes the base test class which can be shared between client and client-sniffer.
Projects that don't depend on elasticsearch-test fail otherwise because org.elasticsearch.test.EsIntegTestCase (default integ test class) is not in the classpath. They should provide their onw integ test base class, but having integration tests should not be mandatory. One can simply set skipIntegTestsInDisguise to true to prevent loading of integ test class.
Unit tests rely on mockito to mock the internal HttpClient instance. No http request is performed, we only simulate interaction between RestClient and its internal HttpClient.
Store default headers ourselves instead, otherwise default ones cannot be replaced. Don't allow for multiple headers with same key, last one wins and replaces previous ones with same key.
Also fail with null params or headers.
Although elasticsearch doesn't support these methods (RestController doesn't even allow to register handler for them), the RestClient should allow to send requests using them.
Use sun HttpServer instead and disable forbidden-apis for test classes. It turns out to be more flexible than okhttp as it allows get & delete with body.
Create a new subproject called client-sniffer that contains the o.e.client.sniff package. Since it is going to go to a separate jar, due to its additional functionalities and dependency on jackson, it makes sense to have it as a separate project that depends on client. This way we make sure that client doesn't depend on it etc.
ElasticsearchResponseException, as well as ElasticsearchResponse, should only be created from o.e.client package.
RequestLogger should only be used from this package too.
Instead of having a Connection mutable object that holds the state of the connection to each host, we now have immutable objects only. We keep two sets, one with all the hosts, one with the blacklisted ones. Once we blacklist a host we associate it with a DeadHostState which keeps track of the number of failed attempts and when the host should be retried. A new state object is created each and every time the state of the host needs to be updated.
We still have a wrapper called RestTestClient that is very specific to Rest tests, as well as RestTestResponse etc. but all the low level bits around http connections etc. are now handled by RestClient.
The only small problem is that the response gets closed straightaway and its body read immediately into a string. Should be ok to load it all into memory eagerly though in case of errors. Otherwise it becomes cumbersome to have an exception implement Closeable...
The connection class can be greatly simplified now that we don't ping anymore. Pings required a special initial state (UNKNOWN) for connections, to indicate that they require pinging although they are not dead. At this point we don't need the State enum anymore, as connections can only be dead or alive based on the number of failed attempts. markResurrected is also not needed, as it was again a way to make pings required. RestClient can simply pick a dead connection now and use it, no need to change its state when picking the connection.
Given that we don't use streams anymore, we can check straightaway if the connection iterator is empty before returning it and resurrect a connection when needed directly in the connection pool, no lastResortConnection method required.
There are two implementations of connection pool, a static one that allows to enable/disable pings, and a sniffing one that sniffs nodes from the nodes info api.
Transport retrieves a stream of connections from the connection for each request and calls onSuccess or onFailure depending on the result of the request.
Transport also supports a max retry timeout to control the timeout for the request retries overall.
javanna
Tanguy Leroux
Yannick Welsch
Nik Everett
Simon Willnauer