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.

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

Enforce Completion Context Limit #38675

Merged
merged 8 commits into from Feb 18, 2019

Conversation

Projects
None yet
5 participants
@TommyWind
Copy link
Contributor

commented Feb 10, 2019

  • Enforcing a maximum number of completion contexts as requested in #32741
  • Closes #32741
@elasticcla

This comment has been minimized.

Copy link

commented Feb 10, 2019

Hi @TommyWind, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

Enforce Completion Context Limit
* Enforcing a maximum number of completion contexts as reqested in #32741
* Closes #32741
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Feb 11, 2019

@matriv matriv added the >enhancement label Feb 11, 2019

@jimczi
Copy link
Member

left a comment

Thanks @TommyWind , I left a comment regarding where to check the limit.

@@ -497,6 +500,7 @@ static void validateTypeName(String type) {
ContextMapping.validateContextPaths(indexSettings.getIndexVersionCreated(), fieldMappers, fieldTypes::get);

if (reason == MergeReason.MAPPING_UPDATE) {
checkCompletionContextsLimit(fieldMappers);

This comment has been minimized.

Copy link
@jimczi

jimczi Feb 11, 2019

Member

I think it would be easier to perform the check directly in CompletionFieldMapper#Builder#build since you have all the needed informations there. We should also not throw an error on old indices (created before 780) since they were created before this breaking change. You can check the created version of the index in the CompletionFieldMapper with BuilderContext#indexCreatedVersion and throw an error if the index has been created on or after 8.0 and a deprecation warning otherwise.

@TommyWind

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

@jimczi thanks for taking a look. :) Moved the logic the way you suggested. Could you take another look please?

@jimczi
Copy link
Member

left a comment

Thanks @TommyWind , the change looks good. Can you add a small note in the docs regarding the new limit?:
https://github.com/elastic/elasticsearch/blob/master/docs/reference/search/suggesters/context-suggest.asciidoc

@TommyWind

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

@jimczi thanks, I added a note to the docs. Please take a look when you have some time. :)

@jimczi
Copy link
Member

left a comment

Thanks for updating @TommyWind . I'll run the tests in our CI and I'll merge your pr if they pass.

@jimczi

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

@elasticmachine test this please

@TommyWind

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

@jimczi Thanks :)

@TommyWind

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2019

jimczi added some commits Feb 18, 2019

@jimczi

jimczi approved these changes Feb 18, 2019

@jimczi

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

@elasticmachine test this please

@jimczi jimczi merged commit c1ab821 into elastic:master Feb 18, 2019

9 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/bwc Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/docs-check Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 18, 2019

Merge remote-tracking branch 'elastic/master' into retention-lease-st…
…ate-file

* elastic/master:
  Remove tests and branches that will never execute (elastic#38772)
  also check ccr stats api return empty response in ensureNoCcrTasks()
  Add overlapping, before, after filters to intervals query (elastic#38999)
  Mute test elastic#38949
  Add remote recovery to ShardFollowTaskReplicationTests (elastic#39007)
  [ML] More advanced post-test cleanup of ML indices (elastic#39049)
  wait for shard to be allocated before executing a resume follow api
  Update track-total-hits.asciidoc
  Force kill testcluster nodes (elastic#37353)
  Make pullFixture a task dependency of resolveAllDependencies (elastic#38956)
  set minimum supported version (elastic#39043)
  Enforce Completion Context Limit (elastic#38675)
  Mute test
  Don't close caches while there might still be in-flight requests. (elastic#38958)
  Fix elastic#38623 remove xpack namespace REST API (elastic#38625)
  Add data frame feature (elastic#38934)
  Test bi-directional index following during a rolling upgrade. (elastic#38962)
  Generate mvn pom for ssl-config library (elastic#39019)
  Mute testRetentionLeaseIsRenewedDuringRecovery

jimczi added a commit to jimczi/elasticsearch that referenced this pull request Feb 18, 2019

Enforce Completion Context Limit (elastic#38675)
This change adds a limit to the number of completion contexts that a completion field can define.

Closes elastic#32741

jimczi added a commit that referenced this pull request Feb 19, 2019

Enforce Completion Context Limit (#38675) (#39075)
This change adds a limit to the number of completion contexts that a completion field can define.

Closes #32741
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.