Add "Async" to the end of each Async RestClient method #20172
Conversation
pickypg
added
>enhancement
:Clients/Java Low Level REST Client
| @@ -98,7 +98,7 @@ protected void doStart(Consumer<? super Response> onResponse) { | |||
|
|
|||
| void lookupRemoteVersion(Consumer<Version> onVersion) { | |||
| execute("GET", "", emptyMap(), null, MAIN_ACTION_PARSER, onVersion); | |||
|
|
|||
|
LGTM, left minor comment |
This makes it much harder to accidentally miss the Response.
pickypg
force-pushed
the
feature/add-async-for-async-calls
branch
from
646b69b
to
bd0b064
Compare
|
@pickypg can you elaborate on how one could miss the response? ResponseListener is a required argument. it's not that we return a future and people can forget to call get against it here... I think we need to update the docs as well: https://www.elastic.co/guide/en/elasticsearch/client/java-rest/current/_performing_requests.html |
Thankfully! It's really just to avoid the confusion. I've used both the sync and async methods in my own code now, and I had to baby-step out the parameters to ensure that I was calling the right one. On the bright side, you also did a good job by making the synchronous variants throw an A synchronous and asynchronous API only differing by parameters is still trappy in my book, particularly if you're unfamiliar with what's happening. I agree that any experienced user wouldn't make these mistakes (in fact, you can't), but those 5 letters go a long way to help guide the end user. |
Good catch. I'll submit a separate PR for that. |
|
fair enough, thanks for the explanation. |
This makes it much harder to accidentally miss the
Response.Closes #20168