Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove unsed fields in RestAnalyzeAction #66215

Merged
merged 2 commits into from
Dec 14, 2020
Merged

Remove unsed fields in RestAnalyzeAction #66215

merged 2 commits into from
Dec 14, 2020

Conversation

chengyang14
Copy link
Contributor

Remove unsed fields in RestAnalyzeAction

@tvernum tvernum added :Search Relevance/Analysis How text is split into tokens >refactoring labels Dec 14, 2020
@elasticmachine elasticmachine added the Team:Search Meta label for search team label Dec 14, 2020
@elasticmachine
Copy link
Collaborator

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

@tvernum
Copy link
Contributor

tvernum commented Dec 14, 2020

@elasticmachine test this please

cbuescher
cbuescher requested changes Dec 14, 2020
View reviewed changes
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

Hi @chengyang14,

thanks a lot for opening this PR and your contribution to Elasticsearch. I looked at the changes and was suprised we don't use the ParseFields here. I saw we now do the parsing of the "_anaylze" request in the ObjectParser defined in class AnalyzeAction. Instead of deleting six of the constants here and leaving the three that are used in CategorizationAnalyzerConfig, I think I would be good to move all constants to AnalyzeAction and use them in the parser definition there and also point to them from CategorizationAnalyzerConfig. Would you mind making those changes?

@cbuescher cbuescher self-assigned this Dec 14, 2020
@chengyang14
Copy link
Contributor Author

Hi @cbuescher
Thanks for your suggestions, I will move all constants to AnalyzeAction.

@cbuescher
Copy link
Member

@elasticmachine test this please

cbuescher
cbuescher approved these changes Dec 14, 2020
View reviewed changes
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, LGTM.
I'll be running our CI test suit before merging.

@cbuescher cbuescher merged commit e8332e1 into elastic:master Dec 14, 2020
cbuescher pushed a commit that referenced this pull request Dec 14, 2020
@chengyang14
Remove unsed fields in RestAnalyzeAction (#66215)
f1a709c 
Moves ParseFields from `RestAnalyzeAction` to `AnalyzeAction` where they
are actually used for parsing the request body.
@cbuescher
Copy link
Member

@chengyang14 thanks for this change, I merged it to master and the 7.x branch.

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Dec 14, 2020
@jasontedor
Merge remote-tracking branch 'elastic/master' into license-to-autoscale
3abdab8 
* elastic/master: (33 commits)
  Add searchable snapshot cache folder to NodeEnvironment (elastic#66297)
  [DOCS] Add dynamic runtime fields to docs (elastic#66194)
  Add HDFS searchable snapshot integration (elastic#66185)
  Support canceling cross-clusters search requests (elastic#66206)
  Mute testCacheSurviveRestart (elastic#66289)
  Fix cat tasks api params in spec and handler (elastic#66272)
  Snapshot of a searchable snapshot should be empty (elastic#66162)
  [ML] DFA _explain API should not fail when none field is included (elastic#66281)
  Add action to decommission legacy monitoring cluster alerts (elastic#64373)
  move rollup_index param out of RollupActionConfig (elastic#66139)
  Improve FieldFetcher retrieval of fields (elastic#66160)
  Remove unsed fields in `RestAnalyzeAction` (elastic#66215)
  Simplify searchable snapshot CacheKey (elastic#66263)
  Autoscaling remove feature flags (elastic#65973)
  Improve searchable snapshot mount time (elastic#66198)
  [ML] Report cause when datafeed extraction encounters error (elastic#66167)
  Remove suggest reference in some API specs (elastic#66180)
  Fix warning when installing a plugin for different ESversion (elastic#66146)
  [ML] make `xpack.ml.max_ml_node_size` and `xpack.ml.use_auto_machine_memory_percent` dynamically settable (elastic#66132)
  [DOCS] Add `require_alias` to Bulk API (elastic#66259)
  ...
@chengyang14 chengyang14 deleted the remove_unused_filed branch December 28, 2020 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbuescher cbuescher cbuescher approved these changes

Assignees

@cbuescher cbuescher

Labels
>refactoring :Search Relevance/Analysis How text is split into tokens Team:Search Meta label for search team v7.12.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@chengyang14 @elasticmachine @tvernum @cbuescher @jakelandis