Skip to content

Async search: rename REST parameters#54198

Merged
javanna merged 4 commits intoelastic:masterfrom
javanna:enhancement/async_search_rename_parameters
Mar 26, 2020
Merged

Async search: rename REST parameters#54198
javanna merged 4 commits intoelastic:masterfrom
javanna:enhancement/async_search_rename_parameters

Conversation

@javanna
Copy link
Copy Markdown
Contributor

@javanna javanna commented Mar 25, 2020 •
edited by lukasolson
Loading

This commit renames wait_for_completion to wait_for_completion_timeout in submit async search and get async search.
Also it renames clean_on_completion to keep_on_completion and turns around its behaviour.

Closes #54069

javanna added 2 commits March 25, 2020 16:36
@javanna
Async search: rename REST parameters
b55fb33 
This commit renames wait_for_completion to wait_for_completion_timeout in submit async search and get async search.
Also it renames clean_on_completion to keep_on_completion and turns around its behaviour.

Closes elastic#54069
@javanna
docs
af51c14
@javanna javanna added >enhancement :Search/Search Search-related issues that do not fall into other categories v8.0.0 v7.7.0 v7.8.0 labels Mar 25, 2020
@javanna javanna requested a review from jimczi March 25, 2020 15:43
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Mar 25, 2020

Pinging @elastic/es-search (:Search/Search)

@cbuescher cbuescher mentioned this pull request Mar 25, 2020
Merged
@bpintea
Copy link
Copy Markdown
Contributor

bpintea commented Mar 25, 2020

7.7 has been FFrozen, can the v7.7.0 label be removed?

@javanna
Copy link
Copy Markdown
Contributor Author

javanna commented Mar 25, 2020

@Mpdreamz can correct me if I am wrong, but I think that this a blocker for the language clients. As far as I understand wait_for_completion is expected to be a boolean like in other API and breaks some of the clients, which is why it was decided to rename it.

@bpintea
Copy link
Copy Markdown
Contributor

bpintea commented Mar 25, 2020 •
edited
Loading

Thanks, @javanna!

@Mpdreamz can correct me if I am wrong, but I think that this a blocker for the language clients.

@jpountz May I update the list of blocking issues to include this one?

@Mpdreamz
Copy link
Copy Markdown
Member

Mpdreamz commented Mar 25, 2020

It won't break the clients (unless it was an enum) but these type of changes are incredibly hard to patch after a release. Since this was discussed and agreed upon by a large group on the issueI would vote to raise it as blocker so that it can go in the next bc. Similar to #54196

@jpountz
Copy link
Copy Markdown
Contributor

jpountz commented Mar 25, 2020

+1 to merge to 7.7. I replaced the >enhancement label with >non-issue since async search has not been released yet.

jimczi
jimczi reviewed Mar 26, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left two comments, LGTM otherwise

@javanna javanna merged commit 1c48214 into elastic:master Mar 26, 2020
javanna added a commit that referenced this pull request Mar 26, 2020
@javanna
Async search: rename REST parameters (#54198)
ff26916 
This commit renames wait_for_completion to wait_for_completion_timeout in submit async search and get async search.
Also it renames clean_on_completion to keep_on_completion and turns around its behaviour.

Closes #54069
@javanna
Copy link
Copy Markdown
Contributor Author

javanna commented Mar 26, 2020

heads up @lukasolson you will have to rename waitForCompletion to waitForCompletionTimeout in your code. I believe Kibana is not using cleanOnCompletion hence no change needed there.

javanna added a commit that referenced this pull request Mar 26, 2020
@javanna
Async search: rename REST parameters (#54198)
2814226 
This commit renames wait_for_completion to wait_for_completion_timeout in submit async search and get async search.
Also it renames clean_on_completion to keep_on_completion and turns around its behaviour.

Closes #54069
@lukasolson lukasolson mentioned this pull request Mar 27, 2020
Merged
@lizozom
Copy link
Copy Markdown

lizozom commented Apr 1, 2020

@lukasolson @javanna was this backported to 7.7?

@javanna
Copy link
Copy Markdown
Contributor Author

javanna commented Apr 1, 2020

@lizozom yes it was, see 2814226

@lizozom
Copy link
Copy Markdown

lizozom commented Apr 1, 2020

@javanna I'm asking, because backporting the kibana change to 7.7 fails in testing, while the 7.x and the 8.0 branches were working and merged

elastic/kibana#61917

@lizozom
Copy link
Copy Markdown

lizozom commented Apr 1, 2020

@javanna I think I might have found the issue. Lets see how the build goes!

@javanna
Copy link
Copy Markdown
Contributor Author

javanna commented Apr 1, 2020

@lizozom I see, let me know if I can do anything to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jimczi jimczi jimczi approved these changes

Assignees

No one assigned

Labels

>non-issue :Search/Search Search-related issues that do not fall into other categories v7.7.0 v7.8.0 v8.0.0-alpha1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Clarify async search REST parameters

8 participants

@javanna @elasticmachine @bpintea @Mpdreamz @jpountz @lizozom @jimczi @jakelandis