Await endpoints in check, not httpClient #2667
Conversation
| * Customizes the {@link HttpClientBuilder} used when connecting to ElasticSearch. Mostly for | ||
| * testing. | ||
| * Customizes the {@link HttpClientBuilder} used when connecting to ElasticSearch. This is used | ||
| * by the server and tests to enable detailed logging and tweak timeouts. |
There was a problem hiding this comment.
this is good but doesnt explain why it needs to be public. usually for tests we use internal package accessors which allows us to remove or change the method. we dont expose public methods unless there is non test code using it in other words
There was a problem hiding this comment.
ex below you could add exactly why like // specifically this is used for self tracing
or whatever it is used for.. reason is, that sometimes we have added methidd for gcp or aws and forgot and then broke them
There was a problem hiding this comment.
The main reason is the server - it calls these from zipkin2.server.internal.elasticsearch. Actually don't see usage of these in zipkin-aws.
I'll leave the comments like this for now since not sure of a better alternative.
|
ps my note on docs is a nit as they are already better than probably everything. just didn't merge as it is red and haven't looked into why |
| * Customizes the {@link HttpClientBuilder} used when connecting to ElasticSearch. Mostly for | ||
| * testing. | ||
| * Customizes the {@link HttpClientBuilder} used when connecting to ElasticSearch. This is used | ||
| * by the server and tests to enable detailed logging and tweak timeouts. |
There was a problem hiding this comment.
The main reason is the server - it calls these from zipkin2.server.internal.elasticsearch. Actually don't see usage of these in zipkin-aws.
I'll leave the comments like this for now since not sure of a better alternative.
|
heh we will have to port zipkin-aws soon (probably today would be good) as it is not using this because it hasn't migrated off okhttp yet https://github.com/openzipkin/zipkin-aws/blob/master/autoconfigure/storage-elasticsearch-aws/src/main/java/zipkin/autoconfigure/storage/elasticsearch/aws/ZipkinElasticsearchAwsStorageAutoConfiguration.java#L53 |
* Await endpoints in check, not httpClient * More explicit cleanup order in autocomplete tests.
Noticed that some non-integration tests were slow waiting and failing on the initial endpoints because they don't have real ones. It was dubious to be waiting in the method that creates an object anyways, so I moved it to
checkwhich seems like the right spot. Reduced the timeout to use the configured one too.Also apply some cleanups mentioned in #2653