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

SCRIPTING: Move terms_set Context to its Own Class #33602

Merged
merged 5 commits into from
Sep 14, 2018
Merged

SCRIPTING: Move terms_set Context to its Own Class #33602

merged 5 commits into from
Sep 14, 2018

Conversation

original-brownbear
Copy link
Member

  • Extracted TermsSetQueryScript
  • Kept mechanics close to what they were with SearchScript

@original-brownbear
SCRIPTING: Move terms_set Context to its Own Class
cd2dc5a 
* Extracted TermsSetQueryScript
* Kept mechanics close to what they were with SearchScript
@original-brownbear original-brownbear added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v7.0.0 >refactoring v6.5.0 labels Sep 11, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

rjernst
rjernst reviewed Sep 11, 2018
View reviewed changes
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

Thanks @original-brownbear. This looks good, except that it is missing backcompat, which I think is important. Eg doc can be accessed through params.doc. We need a way to detect and deprecate these uses. The current idea @jdconrad and I have discussed is a wrapper Map around params that also takes in a set of keys that are deprecated, and the deprecation logger. Then on get calls, the deprecated set can be checked. We could reuse this wrapper Map for converting the other script uses to contexts as well.

@rjernst
Copy link
Member

rjernst commented Sep 11, 2018

We could theoretically not have backcompat for params.doc when that use is not documented. For example, we could use this PR as is, but would still need it for minimum should match scripts because they have special values passed in by the script execution in params, which we need to add as variables in the new context and deprecate their use through params. I would be ok with this, if we get at least a loose consensus from the group first.

@original-brownbear
Copy link
Member Author

@rjernst

The current idea @jdconrad and I have discussed is a wrapper Map around params that also takes in a set of keys that are deprecated, and the deprecation logger. Then on get calls, the deprecated set can be checked. We could reuse this wrapper Map for converting the other script uses to contexts as well.

Jup, the two of us discussed this too on Friday :) I'm fine adding an implementation of this here, it's really not that hard to implement and gives us a general solution to the problem. => go ahead an implement here? :)

@rjernst
Copy link
Member

rjernst commented Sep 11, 2018

Sure, here is great!

@original-brownbear
Copy link
Member Author

@rjernst done, let me know what you think about the approach of passing a map of keys to deprecation messages. I kinda liked this because it would give us a natural way of handling various messages where we want to give additional context like e.g.

elasticsearch/server/src/main/java/org/elasticsearch/action/update/UpdateHelper.java

Line 293 in 79375d3

deprecationLogger.deprecated("Using `ctx` via `params.ctx` is deprecated. " +

rjernst
rjernst approved these changes Sep 13, 2018
View reviewed changes
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM, but I wonder if the map to a full message is needed, or if we could just have the new variable name that should be used. This way the messages can be consistent for params deprecations.

@original-brownbear
Copy link
Member Author

@rjernst thanks!

but I wonder if the map to a full message is needed, or if we could just have the new variable name that should be used

I think in the vast majority of cases just having both variables would probably be enough. Also, I agree that it would be nice to enforce a consistent deprecation message.
On the other hand, it'd also be nice to be able to add more context for cases like the example I mentioned above (which we will potentially have more of in future?).
=> I guess we could just add a static helper to the params map that gives you the standard message for two input keys (old and new variable name) and use that unless we want some custom message?

I'll merge this now so I can use it for another context :) I think we can easily make this change in a follow up PR.

@original-brownbear original-brownbear merged commit 0b4960f into elastic:master Sep 14, 2018
@original-brownbear original-brownbear deleted the replace-term-set-query-context branch September 14, 2018 04:21
@original-brownbear original-brownbear mentioned this pull request Sep 14, 2018
Merged
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Sep 14, 2018
@jasontedor
Merge branch 'master' into global-checkpoint-polling
828b61c 
* master: (24 commits)
  Only notify ready global checkpoint listeners (elastic#33690)
  Don't count hits via the collector if the hit count can be computed from index stats. (elastic#33701)
  Expose retries for CCR fetch failures (elastic#33694)
  Test fix - Graph vertices could appear in different orders based on map insertion sequence (elastic#33709)
  Structured audit logging (elastic#31931)
  Core: Add DateFormatter interface for java time parsing (elastic#33467)
  [CCR] Check whether the rejected execution exception has the shutdown flag set (elastic#33703)
  Mute ClusterDisruptionIT#testSendingShardFailure
  Revert "Mute FullClusterRestartSettingsUpgradeIT"
  Adjust BWC version on settings upgrade test (elastic#33650)
  [ML] Allow overrides for some file structure detection decisions (elastic#33630)
  Adapt skip version for doc_values format deprecation
  [TEST] wait for no initializing shards
  [Docs] Minor fix in `has_child` javadoc comment (elastic#33674)
  Mute FullClusterRestartSettingsUpgradeIT
  [Kerberos] Add realm name & UPN to user metadata (elastic#33338)
  [TESTS] Disable specific locales for RestrictedTrustManagerTest (elastic#33299)
  SQL: Return functions in JDBC driver metadata (elastic#33672)
  SCRIPTING: Move terms_set Context to its Own Class (elastic#33602)
  AwaitsFix testRestoreMinmal
  ...
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Sep 19, 2018
@original-brownbear
SCRIPTING: Add Expr. Compile for TermSetQuery Ctx.
20a66f9 
* Follow up to elastic#33602 adding the ability to compile TermsSetQuery
scripts with the expressions engine in the same way we support
SearchScript in Expressions
   * Duplicated the code here for now to make the change less complex,
 the only difference to SearchScript is that `_score` and `_value` are not handled for TermsSetQuery
@original-brownbear original-brownbear mentioned this pull request Sep 19, 2018
Merged
@rjernst rjernst mentioned this pull request Sep 26, 2018
Merged
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Sep 26, 2018
@original-brownbear
SCRIPTING: Move terms_set Context to its Own Class (elastic#33602)
bc4a76c 
* SCRIPTING: Move terms_set Context to its Own Class

* Extracted TermsSetQueryScript
* Kept mechanics close to what they were with SearchScript
@original-brownbear original-brownbear mentioned this pull request Sep 26, 2018
Merged
original-brownbear added a commit that referenced this pull request Sep 26, 2018
@original-brownbear
SCRIPTING: Move terms_set Context to its Own Class (#33602) (#34068)
03add58 
* SCRIPTING: Move terms_set Context to its Own Class

* Extracted TermsSetQueryScript
* Kept mechanics close to what they were with SearchScript
original-brownbear added a commit that referenced this pull request Oct 4, 2018
@original-brownbear
SCRIPTING: Terms set query expression (#33856)
3ccfc3d 
* SCRIPTING: Add Expr. Compile for TermSetQuery Ctx.

* Follow up to #33602 adding the ability to compile TermsSetQuery
scripts with the expressions engine in the same way we support
SearchScript in Expressions
   * Duplicated the code here for now to make the change less complex,
 the only difference to SearchScript is that `_score` and `_value` are not handled for TermsSetQuery
* remove redundant check
kcm pushed a commit that referenced this pull request Oct 30, 2018
@original-brownbear @kcm
SCRIPTING: Terms set query expression (#33856)
2df110b 
* SCRIPTING: Add Expr. Compile for TermSetQuery Ctx.

* Follow up to #33602 adding the ability to compile TermsSetQuery
scripts with the expressions engine in the same way we support
SearchScript in Expressions
   * Duplicated the code here for now to make the change less complex,
 the only difference to SearchScript is that `_score` and `_value` are not handled for TermsSetQuery
* remove redundant check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjernst rjernst rjernst approved these changes

Assignees
No one assigned
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >refactoring v6.5.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@original-brownbear @elasticmachine @rjernst @colings86