Skip to content

Add _reload_search_analyzers endpoint to HLRC #43733

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jul 3, 2019

Conversation

cbuescher
Copy link
Member

This change adds the new endpoint that allows reloading of search analyzers to
the high-level java rest client.

Relates to #43313

This change adds the new endpoint that allows reloading of search analyzers to
the high-level java rest client.

Relates to elastic#43313
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@cbuescher
Copy link
Member Author

@elasticmachine update branch

@cbuescher
Copy link
Member Author

@elasticmachine update branch

Copy link
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.

code LGTM, I left one comment regarding the shards object that is returned in the response.

--------------------------------------------------
include-tagged::{doc-tests-file}[{api}-response]
--------------------------------------------------
<1> Shard statistics including total number of shards the request was executed on
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not entirely related to this pr but I find it confusing that we return a Shards object in the response even though the action is executed at the node level. I don't have a good way to fix this but we should at least document here that the statistics are per node/index and not per shards ?

Copy link
Member Author

@cbuescher cbuescher Jul 1, 2019

Choose a reason for hiding this comment

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

I agree, this is not ideal. I'll look into the options of either documenting it better, change the naming of the object only for the java api level or whether maybe it makes sense change or even leave out the whole "shard: { ... }". That would have to be done further down the stack though.

Copy link
Member Author

@cbuescher cbuescher Jul 1, 2019

Choose a reason for hiding this comment

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

I looked into the options, and it would be possible but tricky to remove the "shard: { ... }" element from the REST response completely. TransportReloadAnalyzersAction would still be able to extend BroadcastResponse, but we could overwrite "toXContent" without outputing broadcast shards header. Then again, potential failure exceptions are currently also written inside the "shards" header, replacing that toXContent rendering and parsing might turn out to be a bigger change than intended.

Changing the accessor or object name would also involve either not being able to extend BroadcastResponse on the HLRC client side (this would mean more custom parsing code, other getters etc... for failures), so I'll push an update to the docs for now. If you want to discuss the other options again let me know.

Christoph Büscher added 6 commits July 1, 2019 21:04
Currently the repsonse of the "_reload_search_analyzer" endpoint contains the
index names and nodeIds of indices were analyzers reloading was triggered. This
change add the names of the search-time analyzers that were reloaded.

Closes elastic#43804
@cbuescher
Copy link
Member Author

@jimczi as this PR depends on #43813 and CI is on strike right now I cherry-picked that PR in
3578870 (assuming is goes in like this) and added the necessary changes for the additional info returned by the response in the HLRC response class and testing. I think this is ready for another review even though we don't get testing atm.

@cbuescher
Copy link
Member Author

@elasticmachine test this please

@cbuescher
Copy link
Member Author

@elasticmachine run elasticsearch-ci/1

@cbuescher
Copy link
Member Author

@elasticmachine run elasticsearch-ci/2

@cbuescher
Copy link
Member Author

@elasticmachine run elasticsearch-ci/2
@elasticmachine run elasticsearch-ci/packaging-sample

Copy link
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.

LGTM

@cbuescher cbuescher merged commit 1f61152 into elastic:master Jul 3, 2019
cbuescher pushed a commit that referenced this pull request Jul 3, 2019
This change adds the new endpoint that allows reloading of search analyzers to
the high-level java rest client.

Relates to #43313
@jpountz jpountz removed the :Search Relevance/Analysis How text is split into tokens label Jul 5, 2019
@jakelandis jakelandis removed the v8.0.0 label Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants