Clean up HTTP feature support #9
Conversation
| hosts: t.Optional[_TYPE_HOSTS] = None, | ||
| host: t.Optional[_TYPE_HOST] = None, |
There was a problem hiding this comment.
hosts is so pervasive today that I would still allow it to avoid needlessly breaking users trying out serverless by changing from elasticsearch import ... to from elasticsearch_serverless import ...
There was a problem hiding this comment.
I get that. I'm going back and forth on it because it seems important to emphasize that, in Serverless, there is always a one-to-one relationship between the host and the instance.
There was a problem hiding this comment.
Maybe we can support both and only actively document host?
There was a problem hiding this comment.
That could work, but it could be more confusing than helpful. If someone provides a hosts list, does the client pick the first host in the list and log a warning that it's ignoring the rest? Or does it iterate until it finds a host that is healthy? The latter seems more logical (otherwise why support a list at all?), but since it doesn't work just like hosts on the stack client, it may confuse people migrating over who expect it to work the same.
There was a problem hiding this comment.
So I had missed that hosts could be a single string. But believe it or not, many users (including past me) indeed pass a list with a single element: the first page of https://github.com/search?q=Elasticsearch+hosts+language%3APython+&type=code currently shows three such cases.
My proposal is to raise an exception when host and hosts are both passed, and when hosts is a list and contains more than one element. It indeed does not make sense to target multiple hosts in serverless, this is purely for backwards compatibility.
There was a problem hiding this comment.
To be clear it does not have to block this pull request.
There was a problem hiding this comment.
My proposal is to raise an exception when
hostandhostsare both passed, and whenhostsis a list and contains more than one element. It indeed does not make sense to target multiple hosts in serverless, this is purely for backwards compatibility.
I like this. 👍 I'll make a change in this PR before merging.
With clarification that those features are only necessary if connecting to serverless via an external proxy.
docs/guide/connecting.asciidoc
Outdated
| [source,python] | ||
| ---- | ||
| from elasticsearch_serverless import Elasticsearch | ||
|
|
||
| # Adds the HTTP header 'Authorization: ApiKey <base64 api_key.id:api_key.api_key>' | ||
| es = Elasticsearch( | ||
| "https://localhost:9200", | ||
| ca_certs="/path/to/http_ca.crt", | ||
| api_key=("api_key.id", "api_key.api_key") | ||
| ) | ||
| ---- | ||
|
|
There was a problem hiding this comment.
Did you mean to remove this example?
There was a problem hiding this comment.
Yep, it occurred to me that it's effectively identical to the example a few lines up. 😄
There was a problem hiding this comment.
But then the sentence above promises an example that never comes: https://github.com/elastic/elasticsearch-serverless-python/pull/9/files#diff-9e9ed95442744720a1642e738f4845c8acbdf8b7fde0b964b3119c71c643b6b5R45
We should remove "API keys can be provided to the client constructor or via the per-request .options() method:" or add an .options() example.
There was a problem hiding this comment.
Good catch. Fixed in 1af86b2
But only one host in hosts list
Drop support for several HTTP/connection features related to multi-node support:
Also renames
connections_per_nodetoconnectionsfor clarity, enables HTTP compression by default, updates the version number with date metadata, and cleans up the docs to remove references to unsupported HTTP functionality.